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

add prettier as non-code auto formatter #5158

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Jan 4, 2022

Follow-up to #5133 (review)

@facebook-github-bot
Copy link

facebook-github-bot commented Jan 4, 2022

💊 CI failures summary and remediations

As of commit 3f823f1 (more details on the Dr. CI page):


  • 1/1 failures introduced in this PR

1 failure not recognized by patterns:

Job Step Action
CircleCI lint_python_and_config Lint Python code and config files 🔁 rerun

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@NicolasHug
Copy link
Member

Thanks @pmeier ,

if we add this as a pre-commit hook we would also need to add guidelines on how to run it manually. I feel like this would make our contribution process a bit heavy, especially for new contributors. It would also make our contributing guidelines a bit longer, scaring away potential contributors.

Considering the small benefit (IMHO) that this check would bring, I would personally prefer not introducing the checks, and just collectively remember to limit line lengths in files.

@pmeier
Copy link
Collaborator Author

pmeier commented Jan 4, 2022

if we add this as a pre-commit hook we would also need to add guidelines on how to run it manually. I feel like this would make our contribution process a bit heavy, especially for new contributors. It would also make our contributing guidelines a bit longer, scaring away potential contributors.

We only explain ufmt and flake8 in the contributing guide lines. For example

- id: pydocstyle

will be enforced, but is not mentioned. Worse,

- id: check-docstring-first
- id: check-toml
- id: check-yaml
exclude: packaging/.*
- id: mixed-line-ending
args: [--fix=lf]
- id: end-of-file-fixer

are not even executable without pre-commit.

prettier would be in between. It can be executed as an executable, but since it is not a Python package, the contributor would need to setup a JS environment.

Can't we make pre-commit the default way? IIRC, the reason to not do that were slow hooks. We could shorten the contribution guide lines, by explaining how to run with pre-commit and additionally how to skip hooks if need be.

Considering the small benefit (IMHO) that this check would bring

Note that this is not only line-wrapping in markdown files. This also automatically formats yaml and toml files. Previously, we only checked if there were syntactically correct, but didn't enforce an format. With this PR, 20 files would get fixed.

@NicolasHug
Copy link
Member

We don't explain how to use the other checks used in the hooks because those very rarely fail, or are very rarely triggered by most non core-devs.
Describing them in the contributing guidelines would mainly make our guide longer for very little benefit.

Personally I don't feel like formatting yaml or even .md files is such an important priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants