-
Notifications
You must be signed in to change notification settings - Fork 661
chore: add formatting with prettier #1946
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
chore: add formatting with prettier #1946
Conversation
BeksOmega
left a comment
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.
LGTM!
|
After we merge osd branch into master, I'll rebase this, drop the commits that only did formatting and auto-fixing, and then re-run formatting and auto-fixing. Hoping that will avoid most merge conflicts. |
This partially reverts commit cc9914e.
fe624f6 to
a25f77b
Compare
|
@BeksOmega I need to kick the package-locks until they pass in ci (they pass on my machine running but in the meantime this is ready for re-review. i rebased and force pushed so most of the earlier commits are the same. you could view all the commits besides the two marked |
BeksOmega
left a comment
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.
Fixes from last round of comments lgtm!
The basics
The details
Resolves
Fixes #1695
Fixes #962 (with prettier instead of clang-format)
Proposed Changes
Might be easier to review commit-wise. I've summarized here though not in the order of the commits. (sorry)
workspace-minimapplugin had the wrong indentation. no idea why this wasn't fixed.jsdoc/tag-linesrule for now because it was super spammy. will turn it back on and autofix that in a separate PR.formatandformat:checkcommands.blockly-scripts lintscript and associated machinery. We no longer need to invoke eslint through the javascript api. It's simpler to invoke it via command line.eslint-webpack-plugindoesn't support the new format. This was confusing to me anyway. It invoked the linter totally differently than how runningnpm run lintdid which was pretty confusing. Now you won't see lint errors when you runnpm run test. You need to runnpm run lintseparately.Reason for Changes
Also, the old style of eslint config was just, not good. It caused us a lot of problems because technically anyone using a shared config is supposed to also depend on any plugins used by said config. We weren't doing that. That's why some of the typescript plugins had a random dependency on the typescript-eslint parser. Totally confusing that they needed that. Now they don't, because all of the configuration for linting is handled at the root level, while still giving you the option of linting a single plugin.
Test Coverage
Tested that each of the following works:
npm run lintat rootnpm run lintin each pluginnpm run formatat rootnpm run buildat root and in each pluginnpm run testat root and in each pluginDocumentation
We can tell people they can run the formatter now
Additional Information
The field-date tests might be failing? But they're failing on master for me locally too so unrelated.
Holding this until after OSD, then I'll rebase and re-run formatter and eslint fixer.
Future work
@blockly/eslint-configplugin #1975lintscript fromblockly-scripts#1976jsdoc/tag-lineseslint rule #1979