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

Apply prettier, upgrade Node, add PR template #258

Open
wants to merge 6 commits into
base: v0.8.0
Choose a base branch
from

Conversation

FabijanC
Copy link
Contributor

@FabijanC FabijanC commented Dec 3, 2024

  • Supersedes Add format script and run it #241
  • Introduced prettier as devDependency in package.json
    • Replaced .editorconfig with .prettierrc
  • Applied prettier to all .js, .json, .md files.
  • Added formatting commands:
    • npm run format - to be invoked locally
    • npm run format_check - to be invoked as a CI step - updated validate.yml
  • Upgraded node version used in CI from v16 to v20
  • Added .github/pull_request_template.md to serve as a reminder to contributors

@FabijanC FabijanC changed the title Apply prettier, add PR template Apply prettier, upgrade Node, add PR template Dec 3, 2024
@amanusk
Copy link
Collaborator

amanusk commented Dec 19, 2024

Thanks for the much-needed PR

We should merge it as the last PR of 0.8.0 so we don't create too many conflicts with open PRs.

Question: is this with the formatting already applied to all the files? or only partially? (asking because I'm getting file changes after running the scripts)

Also,
What should be the output of format-check? seems to just spit out all of the current files.. Or maybe something is misconfigured for me locally?
I'm not quite sure how prettier behaves with local/global configs, and with local/global .editorconfigs (which is one of the reasons I didn't opt into it right from the start)

@FabijanC
Copy link
Contributor Author

FabijanC commented Dec 19, 2024

Thanks for the review @amanusk!

I'm ok with this being the last PR of 0.8.0

The formatting is already applied, I didn't get any new changes besides the ones resulting from merging with the target branch where my other PR was recently merged and needed formatting. Those changes can be seen in one of my last commits: 4ff04b5. Perhaps the horrible output on npm run format_check led to confusion?

I improved the output of formatting check to make it user friendly and renamed the command from format-check to format_check in this commit: 992b638.

Prettier relies on the locally present .editorconfig. The changes made by your IDE on file save may vary, depending on how the IDE formatting is configured. But even if there are such differences, if the last thing before staging your work and committing is applying the formatting with npm run format, then there should be no fear. To ensure this, I recommend creating an executable file at .git/hooks/pre-commit and adding the following content to it (based on validate.yml):

#!/bin/sh

set -euo pipefail

npm run validate_all
npm run check_component_uniqueness
npm run format_check

That way, all CI checks shall be performed before committing, disallowing the commit if anything fails.

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.

2 participants