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

Extract formatting / lint changes from the Error Recovery PR, and add prettier to ci / local tooling #1500

Merged
merged 6 commits into from
Nov 22, 2023

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Nov 21, 2023

Instead of: #1499

I figured it'd be better to extract the formatting changes from #1462

tl;dr:

  • setup/update eslint / prettier on master
  • squash feature/handle-error to single commit without merge
  • cherry-pick lint/formatting files to feature/handle-error
  • format / fix
  • git rebase origin/master

The way this'll work is we have a one time cost of "run prettier / eslint --fix once" on master, and get things in a predictable spot -- then on the error-recovery branch, we'll rebase / squash down to one commit, copy the lint configs from this branch (as the error recovery branch doesn't have passing lints atm, and some stuff is TODO'd / disabled), then we can run lint:fix over there -- then finally rebase on top of master -- the goal is to reduce the diff from both branches by getting everything formatting / linting the same.

turbo.json Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file was invalid, it's been fixed (had duplicate keys)

@NullVoxPopuli NullVoxPopuli force-pushed the extract-lint-changes-from-error-recovery-PR branch from 3a884bf to 2944439 Compare November 21, 2023 23:32
@NullVoxPopuli NullVoxPopuli force-pushed the extract-lint-changes-from-error-recovery-PR branch from 2944439 to cc3061f Compare November 21, 2023 23:38
@NullVoxPopuli NullVoxPopuli force-pushed the extract-lint-changes-from-error-recovery-PR branch from e34669b to 748445c Compare November 21, 2023 23:40
@NullVoxPopuli NullVoxPopuli mentioned this pull request Nov 21, 2023
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review November 21, 2023 23:43
@@ -21,9 +21,10 @@
"build:control": "rollup -c rollup.config.mjs",
"build:flags": "RETAIN_FLAGS=true ember build --env production --suppress-sizes",
"lint": "npm-run-all lint:*",
"lint:format": "prettier -c .",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatting wasn't being checked on C.I.... 🙃

Copy link
Contributor

@chancancode chancancode left a comment

Choose a reason for hiding this comment

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

👍 on doing this separately in a single pass, obviously I can’t/didn’t review every file, but as long as this is all automated and without code changes then let’s do it!

@NullVoxPopuli NullVoxPopuli merged commit c66e4dd into master Nov 22, 2023
5 checks passed
@NullVoxPopuli NullVoxPopuli deleted the extract-lint-changes-from-error-recovery-PR branch November 22, 2023 00:25
@wycats
Copy link
Contributor

wycats commented Nov 22, 2023

This is awesome! Thanks so much @NullVoxPopuli.

For future reference, the current formatting rules assume that you're running the equivalent of eslint --fix and then prettier -w to get the right formatting.

The eslint part does a lot of heavy lifting around imports and exports. Some of that heavy lifting is pure formatting (sorting, grouping, etc.) but eslint also enforces rules that mandate the use of import type when the import itself is unused as a value. This is essential to ensure that the code actually runs as intended at runtime and is also enforced (without fix support) via the verbatimModuleSyntax tsconfig setting.

In vscode, the expectation is that saving a file will automatically run both eslint fixes and prettier. The settings checked in to .vscode/settings.json, assuming the relevant extensions are installed, should make this happen automatically.

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.

3 participants