protofire / solhint

Solhint is an open-source project to provide a linting utility for Solidity code.
https://protofire.github.io/solhint/
MIT License
1.04k stars 160 forks source link

Drop requirement of double quotes in string literals #316

Closed CodeSandwich closed 1 year ago

CodeSandwich commented 3 years ago

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 commented 2 years ago

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

dbale-altoros commented 1 year ago

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

CodeSandwich commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

https://github.com/protofire/solhint/pull/485