kolibri
kolibri copied to clipboard
Linting updates
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
idunique - Ensuring
imgsrcattribute is not empty - Enforcing
kebab-casecompatibleclassnames - All other rules should be enforced by running prettier on the entire Single File Component
- Ensuring
- Simplifies the
lintfunction inlint.jsby always runningprettieron all files, runningeslintonjsandvuefiles, 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 (
yarnandpip) - Documentation is updated
- Contributor is in AUTHORS.md
Build Artifacts
| Asset type | Download link |
|---|---|
| PEX file | kolibri-.pex |
| Windows Installer (EXE) | kolibri-0.17.0a0.dev0+git.100.gcdea6476-windows-setup-unsigned.exe |
| Debian Package | kolibri_0.17.0a0.dev0+git.100.gcdea6476-0ubuntu1_all.deb |
| Mac Installer (DMG) | kolibri-0.17.0a0.dev0+git.100.gcdea6476-0.4.2.dmg |
| Android Package (APK) | kolibri-0.17.0a0.dev0+git.100.gcdea6476-0.1.3-debug.apk |
| TAR file | kolibri-0.17.0a0.dev0+git.100.gcdea6476.tar.gz |
| TAR file | kolibri-0.17.0a0.dev0+git.100.gcdea6476.tar.gz |
| TAR file | kolibri-0.17.0a0.dev0+git.100.gcdea6476.tar.gz |
| WHL file | kolibri-0.17.0a0.dev0+git.100.gcdea6476-py2.py3-none-any.whl |
| WHL file | kolibri-0.17.0a0.dev0+git.100.gcdea6476-py2.py3-none-any.whl |
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.
I didn't get to read through the new rules but can see they all have test files so feeling confident
Also, I'm not sure if this helps, but noticed that https://github.com/eslint/eslint/issues/3458 has some https://github.com/eslint/eslint/issues/3458#issuecomment-1239811334:
They closed it about a day or two after I last looked at it! Great, I'll update accordingly!
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.
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).
Filed a follow up issue for eslint/vue updates here: https://github.com/learningequality/kolibri/issues/12199
let's go ahead with this this week as we are planning to move to our new branches approach, I'll approve once the merge conflicts are resolved