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

Drop requirement of double quotes in string literals #316

Closed
CodeSandwich opened this issue Nov 2, 2021 · 7 comments
Closed

Drop requirement of double quotes in string literals #316

CodeSandwich opened this issue Nov 2, 2021 · 7 comments

Comments

@CodeSandwich
Copy link

CodeSandwich commented Nov 2, 2021

Solidity allows double or single quotes in string literals. It's useful in cases where the string itself contains quotes, it removes the need for escaping them. Banning one flavor upfront doesn't seem reasonable, it fights usage of a genuine readability improvement tool. Single quotes aren't confusing for the readers and AFAIK don't pose any threat of introducing bugs.

If avoiding single quotes is important, maybe they could be allowed only if the string contains double quotes?

@Ding-Fan
Copy link

JS has backtick but solidity don't. I have to escape every double quote or // solhint-disable-line everywhere.

@dbale-altoros
Copy link
Collaborator

I understand the problem.
Sorry if this seems like a dumb suggestion but:
This rule only checks the type of quotes you are configuring in .solhint.json as
{ rules: { quotes: ['error', 'double'] } } ==> checks for double quotes
{ rules: { quotes: ['error', 'single } } ==> checks for double quotes

If you are using both types of quotes, chose the most useful for you, config that type of quote in .solhint.json and whenever you need to use the other one just disable the rule with a comment...
// solhint-disable-next-line quotes

If there's no 'most useful' and you need both, just disable the rule
Is that makes sense ?

Sorry for the late response.
There is a new release available.
Tomorrow there will be another one.
And we will be updating solhint on regular basis with a dedicated team to it
Feel free to comment or open an issue if you have one

Thanks for posting

@dbale-altoros dbale-altoros added the awaiting user feedback awaiting user feedback label Aug 10, 2023
@CodeSandwich
Copy link
Author

Thanks for the tip, it may be a good solution for many users!

I still think that the defaults could be improved, and that making the rule "smarter" would prevent a lot of frustration. What you're suggesting doesn't make the linter more elastic, the users still need to wrestle with it in some cases, it's just an inverses of when the struggle will happen. Choosing neither double nor single solves the dilemma of whether you should stick to one flavor of quotes, and always escape it in strings, or litter code with solhint-disable-next-lines.

Making the rule look at the content of the string, and automatically disabling errors coming from using quotes in the other flavor, would be the sane default. It would push users to using one flavor consistently (double is a fine choice), and would promote the usage of the other flavor when it makes the code cleaner.

Full disclaimer: I don't use Solhint now.

@dbale-altoros
Copy link
Collaborator

@CodeSandwich thanks for the feedback
So, what you are suggesting (if I understand it correctly) is
If I have this config:
{ rules: { quotes: ['error', 'double'] } } ==> checks for double quotes

Skip the warning for cases like this:
const WHATEVER = 'User the "quotes" now'
So if solhint is set to warn for double quotes, skip it when they are inside single quotes ?
And viceversa ?

On the other hand. If you're still doing solidity, check the new version !! It has a lot of improvements!
Today I will release a newer one at the end of my day

Hey! Thanks a lot for posting and sorry the late response

@CodeSandwich
Copy link
Author

Yes, that would be it! WDYT, do you like this idea? It wouldn't be triggered by the existing code, the linter would only become more forgiving.

Thank you for maintaining Solhint, you're doing an invaluable work! It's a solid tool used all over the ecosystem, and you've effectively revived it.

@dbale-altoros
Copy link
Collaborator

It is a good one... and I understand your point... it is a bummer to disable rules on some lines because of it
Maybe I can put it in the next release

Thanks for your input and your kind words!!!

@dbale-altoros dbale-altoros added rule update and removed awaiting user feedback awaiting user feedback labels Aug 11, 2023
@dbale-altoros
Copy link
Collaborator

#485

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants