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

Increase ESLint requirements for new files #1846

Merged
merged 13 commits into from
Apr 3, 2023
Merged

Conversation

Pezmc
Copy link
Contributor

@Pezmc Pezmc commented Mar 16, 2023

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 with npm 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

  • I have read the contribution guidelines
  • [-] Suitable unit/system level tests have been added and they pass
  • [-] Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • [-] Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • [-] Backport needed? -> add the backport label
  • [-] Includes a DB migration? -> add the area:migration label

@Pezmc Pezmc marked this pull request as draft March 16, 2023 09:54
@Pezmc
Copy link
Contributor Author

Pezmc commented Mar 16, 2023

734d71a has failures not seen locally, downgrading to a draft.

@Pezmc Pezmc force-pushed the chore-eslint-rule-increase branch from 4b42ddd to 9209734 Compare March 20, 2023 09:37
@Pezmc Pezmc marked this pull request as ready for review March 21, 2023 08:27
@Pezmc
Copy link
Contributor Author

Pezmc commented Mar 22, 2023

@Steve-Mcl @knolleary @joepavitt This is ready for review!

Copy link
Contributor

@Steve-Mcl Steve-Mcl left a 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.

image

@Pezmc
Copy link
Contributor Author

Pezmc commented Mar 23, 2023

Thanks @Steve-Mcl, I'll be rebasing this branch on top of main just before it merges to ensure we don't have failures for files currently on other branches.

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.

Copy link
Member

@knolleary knolleary left a 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.

@Steve-Mcl
Copy link
Contributor

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?

@Pezmc
Copy link
Contributor Author

Pezmc commented Mar 23, 2023

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 --fix, I'd suggest it was very low priority here.

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 npx eslint -c .eslintrc -f overrides --ext .js,.vue "**" referenced in 9209734, but there shouldn't be a need to update the list once it's set.

@Pezmc
Copy link
Contributor Author

Pezmc commented Mar 29, 2023

@knolleary @Steve-Mcl Would like to get this merged, did you have any follow up questions?

/cc: @joepavitt @hardillb

@Pezmc Pezmc requested review from Steve-Mcl and hardillb April 3, 2023 11:04
@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented Apr 3, 2023

@Pezmc I do fully agree we should not simply --fix everything in one go however, we should (IMHO) actively progress the removal of exceptions to get the whole of the code base up to a higher standard. Some options/approaches might be to:

  • have a rule whereby if you work on a file that is present in the list, you remove it from the list and fix up any lint issues as part of that work.
    • Perhaps we consider adding a checkbox or reminder to the git template?
  • have a bunch of open issues Have an issue with sub tasks that target sections of the code base
    • To ensure this does not get left behind.

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.

Copy link
Contributor

@hardillb hardillb left a 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

@Pezmc Pezmc force-pushed the chore-eslint-rule-increase branch from 9209734 to 1e4f255 Compare April 3, 2023 12:30
@Pezmc
Copy link
Contributor Author

Pezmc commented Apr 3, 2023

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.

@Pezmc Pezmc merged commit ca2bc0a into main Apr 3, 2023
@Pezmc Pezmc deleted the chore-eslint-rule-increase branch April 3, 2023 14:48
@Pezmc Pezmc mentioned this pull request Apr 3, 2023
9 tasks
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.

None yet

4 participants