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

Linting updates #9698

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Sep 7, 2022

Summary

  • Upgrades all linting related packages in kolibri-tools
  • Adds @rushstack package to ensure eslint module resolution from kolibri-tools (more details here: https://github.com/microsoft/rushstack/tree/main/eslint/eslint-patch#what-it-does)
  • Creates eslint linting rules to replace our previous component line spacing rules (2 spaces between blocks, 1 empty space at the beginning and end of a populated block)
  • Drops HTMLHint completely
  • Creates eslint linting rules to replace our previous use of HTMLHint (rules used):
    • Ensuring id unique
    • Ensuring img src attribute is not empty
    • Enforcing kebab-case compatible class names
    • All other rules should be enforced by running prettier on the entire Single File Component
  • Simplifies the lint function in lint.js by always running prettier on all files, running eslint on js and vue files, and then stylelint on style files and vue components.
  • This PR deliberately does not implement prettier formatting with the eslint-prettier-plugin, as it reduces performance and can lead to more squiggly red lines in IDEs that will later be autofixed.
  • Ensures consistent reporting by comparing source with formatted results and reporting an additional error if it is different.

Note: This does not lint any pre-existing files to limit the diff at the moment (and to prevent massive merge conflicts).

References

Fixes #5410
Supersedes #9660 and supersedes #9689

Reviewer guidance

It seems that the prettier upgrade does slightly change how whitespace around functions is handled which will cause a lot of changes to a lot of files.

For all other breaking changes that I could identify (and which had options) I set the option to induce the fewest changes.

@MisRob one last question - the latest eslint-vue integrates your unused props PR (with support for methods, props, computed etc) - so this might be an opportunity to switch out for mainstream, but I did not attempt that as yet.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added work-in-progress Not ready for review TODO: needs review Waiting for review labels Sep 7, 2022
@rtibbles rtibbles added this to the 0.16.0 milestone Sep 7, 2022
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall code looks good. I didn't manually test it - but if tests pass I suspect it should be all set.

@rtibbles rtibbles force-pushed the linting_updates branch 2 times, most recently from f1b438c to 87ec509 Compare September 12, 2022 22:10
@MisRob
Copy link
Member

MisRob commented Sep 23, 2022

Thank you, @rtibbles. Overall, it's looking good to me. The new lint file is much simpler than it used to be, I appreciate it (and your patience, this task looks like no small undertaking).

@MisRob one last question - the latest eslint-vue integrates your unused props PR (with support for methods, props, computed etc) - so this might be an opportunity to switch out for mainstream, but I did not attempt that as yet.

Sounds good. There are already lots of useful updates in this PR, so whatever works for you.

Also, I'm not sure if this helps, but noticed that the issue that motivated rushstack/eslint has some recent updates from two weeks ago:

Hi folks! Just an update that the new ESLint configuration has now been enabled as an experiment. This new config system eliminates the special treatment of shared configs so that you can include plugins as direct dependencies. Read about it here:

https://eslint.org/blog/2022/08/new-config-system-part-1/
https://eslint.org/blog/2022/08/new-config-system-part-2/
https://eslint.org/blog/2022/08/new-config-system-part-3/

This is the long-term solution to the problem in this issue. We know it took a while, but this was a really complex problem that we needed to think through.

Because we now have this solution implemented, I'm closing this issue. Please try out the new config system and leave your feedback in the discussions.

@MisRob
Copy link
Member

MisRob commented Sep 23, 2022

I didn't get to read through the new rules but can see they all have test files so feeling confident

@rtibbles
Copy link
Member Author

Also, I'm not sure if this helps, but noticed that eslint/eslint#3458 has some eslint/eslint#3458 (comment):

They closed it about a day or two after I last looked at it! Great, I'll update accordingly!

@rtibbles
Copy link
Member Author

They closed it about a day or two after I last looked at it! Great, I'll update accordingly!

I looked - I think it might take quite a bit more work to do this migration to their new flat configuration, so I think I'll leave this for now, and open a follow up issue, once this is merged.

@rtibbles rtibbles removed work-in-progress Not ready for review SIZE: large labels May 16, 2024
@rtibbles rtibbles marked this pull request as ready for review May 16, 2024 03:07
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some questionable indentation changes with comments, particularly inconsistent, and switch blocks that do not make much sense

@rtibbles
Copy link
Member Author

Have fixed what seemed to be the underlying causes of the comment and switch case indentations.

I think this is mostly ready to go, but I'd like to hold off on a final merge with one last rebase after the 0.16 into develop merge PR is merged (we may need to do two of these, as there is one open issue for 0.16.2 that might need a frontend tweak).

@rtibbles
Copy link
Member Author

Filed a follow up issue for eslint/vue updates here: #12199

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: frontend DEV: renderers HTML5 apps, videos, exercises, etc. DEV: tools Internal tooling for development SIZE: large SIZE: very large TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify linting process
5 participants