-
-
Notifications
You must be signed in to change notification settings - Fork 13
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: format all files #22
Conversation
"*.js": [ | ||
"eslint --fix", | ||
"prettier --write" | ||
] | ||
], | ||
"!(*.js)": "prettier --write --ignore-unknown" |
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.
Prettier runs on all files, but separate "*.js"
and "!(*.js)"
tasks are needed to make sure that eslint
runs before prettier
on .js files.
https://github.com/lint-staged/lint-staged?tab=readme-ov-file#task-concurrency
@@ -8,7 +8,8 @@ | |||
"build": "node scripts/build.js", | |||
"lint": "eslint .", | |||
"lint:fix": "eslint --fix .", | |||
"fmt": "prettier --write ." | |||
"fmt": "prettier --write .", |
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.
this should rather be fmt:fix
and the check should be fmt
?
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.
Formatting is currently not covered by our package-json-conventions.
@nzakas what do you think about script names?
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.
I'd suggest lets keep the script name to be format
for analysing formatting problems. and format:fix
for the write formatted changes. I can send a PR to add it to package conventions as well.
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.
format
& format:fix
would be aligned with lint
& lint:fix
, which looks good for consistency, but in the context of the word "format" it might be surprising that format
only checks the format, so maybe format:check
& format:fix
?
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.
Since this PR has a high potential of running into merge conflicts with any future PRs, what do you think about merging this now as-is, and continuing this discussion on a PR that updates package conventions? Then, we could get back to this repo and update the script names if needed.
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.
Yea lets merge this PR and then pick this as a separate issue.
"somePlugin/rule-name": "error" | ||
} | ||
} | ||
{ |
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.
prettier uses tabs by default? Seems like too much space from the existing one?
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.
Yes, tabs are default in Prettier. - wrong
This is only 1 tab, but GitHub by default renders tabs as 8 spaces so it looks huge.
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.
GitHub by default renders tabs as 8 spaces
It's configurable:
I've just changed my Tab size preference to 4 and this looks the same as before on my screen:
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.
Actually, prettier's default indentation is 2 spaces. But we configured it in this project to use tabs:
Line 2 in 3e9eb67
useTabs: true, |
I think just |
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. Just leaving open until @harish-sethuraman's questions are answered.
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
This PR:
npm run fmt
) so that unrelated formatting changes no longer appear in PRs.Verify Files
CI job that checks if all files are lint-free (eslint) and properly formatted (prettier).Question: I assumed we want to format all files that prettier knows to handle. Are there some files we'd like to ignore or format differently? I only ignored
CHANGELOG.md
files because those are autogenerated.