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

chore: add formatting with prettier #1946

Merged
merged 19 commits into from
Oct 26, 2023
Merged

Conversation

maribethb
Copy link
Contributor

@maribethb maribethb commented Sep 22, 2023

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)

  • Switches eslint config format to the new flat config style. See improve eslint config and linting experience #1695 for more background on this. This was a requirement to get Prettier working, due to the complex nature of the old shared config
    • Doing this exposed some files that weren't previously being linted. Some of these, I have no idea why they weren't. e.g. the workspace-minimap plugin had the wrong indentation. no idea why this wasn't fixed.
    • Largely kept the rules the same, other than rules that conflict with Prettier. I also changed some of the jsdoc rules to more closely match what we have in core (e.g. turned off auto-fixing for the rule that was adding blank jsdoc comments)
    • Disabled the jsdoc/tag-lines rule for now because it was super spammy. will turn it back on and autofix that in a separate PR.
  • Installs Prettier and adds format and format:check commands.
  • Removes the blockly-scripts lint script and associated machinery. We no longer need to invoke eslint through the javascript api. It's simpler to invoke it via command line.
  • Removes linting from webpack. eslint-webpack-plugin doesn't support the new format. This was confusing to me anyway. It invoked the linter totally differently than how running npm run lint did which was pretty confusing. Now you won't see lint errors when you run npm run test. You need to run npm run lint separately.
  • Auto formatted and auto-fixed lint problems in the whole repo
  • Manually fixed two actual lint errors that were in a file not previously being linted

Reason for Changes

fmt

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 lint at root
  • npm run lint in each plugin
  • npm run format at root
  • npm run build at root and in each plugin
  • npm run test at root and in each plugin

Documentation

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

@maribethb maribethb requested a review from BeksOmega September 22, 2023 02:16
@maribethb maribethb requested a review from a team as a code owner September 22, 2023 02:16
Copy link
Contributor

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

LGTM!

@maribethb
Copy link
Contributor Author

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.

@maribethb
Copy link
Contributor Author

@BeksOmega I need to kick the package-locks until they pass in ci (they pass on my machine running npm ci so I gotta poke it a bit until they work)

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 autofix lint problems and format everything to see all non-automated changes. one thing i added since last review is in the last commit, excluding the copy of sample-app that might end up in your version of dev-create if you've ever run npm pack in that plugin like i did today...

Copy link
Contributor

@BeksOmega BeksOmega left a 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!

@maribethb maribethb merged commit 82f1c35 into google:master Oct 26, 2023
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.

improve eslint config and linting experience Enable clang-format in samples
2 participants