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

Fix: broken Prettier configuration #850

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ryami333
Copy link
Contributor

In package.json there was a script definition called prettier which mapped to prettier . --write. This meant that whenever you would run pnpm prettier . --check the script would actually be evaluated as prettier . --write "." "--check", and as such pnpm prettier . --check would never fail, because it would always just write the files in place.

This means that a handful of files in the project have already been merged with incorrect formatting because the pnpm run prettier . --check step in the test workflow was ineffective.

@@ -5,23 +5,23 @@

version: 2
updates:
- package-ecosystem: "pip"
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 .github directory had been added to the .prettierignore file, so I removed it and formatted these files.

- run: pnpm run prettier . --check
- run: pnpm prettier . --check
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using pnpm run implies use of a package.json script - whereas in this context we want the binary (we don't want to run prettier . --write).

pnpm-lock.yaml
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pnpm-lock.yaml is generated automatically by pnpm, and its formatting isn't prettier-compliant - we don't want to force developers to format it though.

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.

1 participant