Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(README.md): include HardCodedString linter #278

Merged

Conversation

francisfuzz
Copy link
Contributor

Summary

Closes #254

This updates the README's Linters section to include the HardCodedString linter.

I verified that this linter is not enabled by default as it's configured in this part of the lib/erb_lint/runner_config.rb. The description is sourced from the pull request's OP that originally introduced the linter.

Copy link
Member

@etiennebarrie etiennebarrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think there's no need for a paragraph with bad/good examples like other linters have?

@francisfuzz
Copy link
Contributor Author

Do you think there's no need for a paragraph with bad/good examples like other linters have?

@etiennebarrie - Great question! Personally, including an example would improve this PR. When I used this for my project, my team discovered that we need to provide a custom corrector and a load_path, which does the heavy lifting of actually correcting the offenses. I wish we knew about that ahead of time! 😅

Thinking aloud: could a "good" example be including a custom corrector to work with this linter?

Originally, I refrained from providing one because I did not find any guidelines for repository contributors for requiring one and I noticed that not all linters have examples (like ClosingErbTagIndent and ExtraNewline).

@etiennebarrie
Copy link
Member

Can you add one for this one? Especially if you say it's particularly useful. We can add the other ones later.

@francisfuzz
Copy link
Contributor Author

@etiennebarrie - Thanks for asking! I started in 1607aac. I'd like to verify what I wrote down works in a test repository before asking for another review.

Copy link
Member

@etiennebarrie etiennebarrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to verify what I wrote down works in a test repository before asking for another review.

Did you get a chance to try this in a test repository?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@francisfuzz
Copy link
Contributor Author

I'd like to verify what I wrote down works in a test repository before asking for another review.

Did you get a chance to try this in a test repository?

👋 @etiennebarrie - Hi there! I'll be honest: this slipped from my radar, sorry!

However, I have some time today to work on this! I created https://github.com/francisfuzz/erb-lint-playground to test things out. Thanks for your suggested changes -- I'll go ahead review+accept those now.

francisfuzz and others added 2 commits June 9, 2023 09:36
Co-authored-by: Étienne Barrié <[email protected]>
Co-authored-by: Étienne Barrié <[email protected]>
@francisfuzz
Copy link
Contributor Author

Update! I've been working with my company internally regarding the CLA before continuing this work -- I'll follow up once I get an update from them. 🙇

@samrjenkins
Copy link

I just ran into this issue. Would be awesome to get this merged. Had me confused for a few minutes!

Copy link
Member

@etiennebarrie etiennebarrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the code block. Thanks for your contribution!

@etiennebarrie etiennebarrie merged commit ab1f55d into Shopify:main Sep 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The HardCodedString linter is not documented
3 participants