Skip to content

Add Black and other auto formatters as pre-commit hooks #1534

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

Closed
wants to merge 4 commits into from

Conversation

abhinavarora
Copy link
Contributor

@abhinavarora abhinavarora commented Jan 21, 2022

This PR sets up pre-commit hooks for auto-formatting files in the repository. This includes black and usort and are designed to be compatible with Meta's internal workflow.

How to set up locally

  1. To install the hooks pip install pre-commit
  2. Run pre-commit install inside the repository
  3. Now, every time you git commit something, these hooks will run and make sure the code format is correct.
  4. You can also invoke them manually with pre-commit run, which will check all staged files

@erip
Copy link
Contributor

erip commented Jan 21, 2022

The black reformat will be quite a large diff, but I suppose it should also be done here? If so, we should probably hold off on merging this until "active" PRs are merged since big reformats are PR killers.

@abhinavarora
Copy link
Contributor Author

The black reformat will be quite a large diff, but I suppose it should also be done here? If so, we should probably hold off on merging this until "active" PRs are merged since big reformats are PR killers.

Hey @erip This PR just adds the configs for linting in CI. I am planning to create a subsequent PR over the weekend that formats all the files.

@abhinavarora
Copy link
Contributor Author

Current test failures seem to be unrelated to this PR as they come from datasets. Lint will now fail in CI. I plan to create a subsequent PR that formats all the files.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Looks good @abhinavarora !

I saw you checked that arc lint is happy with the related changes in D33838656, which is great.

I assume this is left for a follow-up PR, but you will probably want to update the contributing guidelines as well

- usort == 0.6.4


- repo: https://github.com/pre-commit/mirrors-clang-format
Copy link
Member

Choose a reason for hiding this comment

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

Nit / Question / Suggestion

I'm not familiar with torchtext CI job details, but this is possibly redundant with the run_style_checks.sh job? You could also have a flake8 hook done here (like in torchvision) and possibly remove run_style_checks.sh entirely.



- repo: https://github.com/pre-commit/mirrors-clang-format
rev: v13.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this clang compatible with the one PyTorch is using?

PyTorch core uses specific binaries so that the result is consistent across platforms.

https://github.com/pytorch/pytorch/blob/24b60b2cbf37f46818624f36df2e539f5b187a88/tools/linter/adapters/s3_init_config.json#L2-L15

Copy link
Contributor

Choose a reason for hiding this comment

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

well, as long as it's consistent within torchtext, then there should not be an issue

@abhinavarora
Copy link
Contributor Author

Thanks for the review @NicolasHug and @mthrok . I will close this diff in favor of #1546 by @pmeier which is more elaborate than mine :-)

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.

5 participants