-
Notifications
You must be signed in to change notification settings - Fork 63
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
Increase ESLint requirements for new files #1846
Conversation
734d71a has failures not seen locally, downgrading to a draft. |
4b42ddd
to
9209734
Compare
@Steve-Mcl @knolleary @joepavitt This is ready for review! |
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.
@Pezmc - loaded onto windows machine & tried creating new files etc - as expected, tighter linting rules were shown for new files (e.g. copying many of the existing VUE files showed numerous new warnings and errors) 👍
1 Question I have is:
Most of the linting markers are only "warnings". Is this deliberate or should we be stricter?
E.g. enforcing order of directives makes for a familiar VUE file layout.
Thanks @Steve-Mcl, I'll be rebasing this branch on top of I've left Vue.js style issues as warnings only rather than errors, that is to say, they don't cause problems, but ideally we'd fix them up. The majority of the warnings can be auto-fixed, but not all, so it felt best to leave it as a warning, rather than a hard requirement. |
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.
My concern here is we now have over 1000 lines of exceptions hardcoded in the lint file. Is the intent to slowly chip away at those in the background when those files get touched for other work that is in plan?
The file says that list is autogenerated - how was that generated? Would be helpful to have that documented in the PR for future reference.
I had a similar question to @knolleary on what our intent was following this. Perhaps we raise an issue (or several) to do "sections" of the code base to reduce this list of files? |
The very long term objective would be to clear the list of exceptions, but given that it doesn't bring that much value just yet, and is likely to make a mess of git history if run using The key objective here is to hold new files to higher standards, rather than being forced to use lower standards because of the older files in the codebase. @knolleary The overrides were generated using |
@knolleary @Steve-Mcl Would like to get this merged, did you have any follow up questions? /cc: @joepavitt @hardillb |
@Pezmc I do fully agree we should not simply
Otherwise, as stated before, in my testing, this was working as expected. So long this doesnt just become technical debt & we have plans to whittle this down, I personally approve. |
This is already set by `"es2022": true`
Checks imports are in a sensible order, and that they resolve properly (only for ESM files)
``` npx eslint -c .eslintrc -f overrides --ext .js,.vue "**" ```
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'll say yes to this, but I think we should have a tax to pull the exceptions down.
e.g. an issue per release to remove one of the exception blocks
9209734
to
1e4f255
Compare
In the interests of maintaining bias for action, I'm merging this PR, we can continue to finesse the best way to keep track of the list (see @Steve-Mcl's comment above) on this thread. |
Do not merge after review: I'll merge main into this branch first to ensure it's up to date.
Description
This PR increases our ESLint rule requirements in two ways:
Importantly these new rules only apply to new files, that is to say, any file that doesn't meet the new higher standard, now has an override set in the ESLint.
This means that new files are held to a higher standard, they're more consistent and more bugs and patterns likely to lead to errors are caught, while still allowing the old failing files to exist.
The significant majority of the new rules all support
eslint --fix
, which should be enabled in your editor as per the handbook or run manually withnpm run lint:fix
.Impact to team: Theoretically zero if auto-fix is enabled in your editor
Impact to codebase: Stricter standards catch bugs earlier and improve consistency making work faster
Related Issue(s)
None
Checklist
flowforge.yml
?flowforge/helm
to update ConfigMap Templateflowforge/CloudProject
to update values for Staging/ProductionLabels
backport
labelarea:migration
label