-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Improve JS linting #3212
Improve JS linting #3212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! Just left one comment
.eslintrc.json
Outdated
"no-console": ["warn", { | ||
"allow": ["warn"] | ||
}], | ||
"no-extra-semi": "warn", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a warning instead of an error for this rule? Do we expect to use extra semicolons during development?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the discussion in #2847, I think the general consensus is to keep semicolons and only remove the ones that are completely unnecessary.
As for the warning instead of an error, I think that was an arbitrary decision 😅 I feel like the warnings should be used for stuff like this, whilst errors should be used when variable shadowing, unused vars or no var
usage issues are present, but I'm completely willing to discuss what's the better choice for this. Let me know what you think! 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://eslint.org/docs/rules/no-extra-semi
I think I'm reading the same way as @mdesantis. This is a non-blocker for me, but I would go ahead and make it an error personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, @mdesantis. Fixing it as we speak! 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks @aitbw!
This rule does not raise any offenses within Solidus codebase, making it safe to enable
We can specify ESLint environments to pre-define global JS variables not defined by Solidus; this way we can whitelist several vars found within the project's codebase without the need to specify them one by one See https://eslint.org/docs/user-guide/configuring#specifying-environments for details
ESLint will now check for all unused variables, regardless of their scope, but won't do so for function arguments
Solidus is not a JavaScript project, it does not makes much sense to enforce ES6+ syntax
c06e485
to
af8f6f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aitbw 👕thank you!
Thanks @aitbw, you are great! |
Description
Following #3167, this PR aims to further improve JS linting by enabling non-offending rules, taking advantage of ESLint environments and whitelisting backend-specific global variables.
Checklist: