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

Introduce pre-commit with pre-commit suggestions #4972

Closed
wants to merge 8 commits into from
Closed

Conversation

SeanNaren
Copy link
Collaborator

@SeanNaren SeanNaren commented Sep 21, 2022

What does this PR do?

Currently style checking is done within the setup.py and enforced via jenkins.

This is problematic for users who do not have jenkins access and builds fail due to simple style fixes. By using pre-commit CI, we can automate style fixes in PRs meaning less work for users (and have a dedicate CI job for this).

As a side effect of this, due to the old version of black running I had to upgrade the Black version from the pre-release (19) to a stable version (22.*). Due to this there has been some additional style changes. A few summarized below:

  • Lots of whitespace removed
  • Multiple argument functions are expanded based on length. this is primarily for readability (and hence why the number of lines go up):

i.e

# old
{'label': NeuralType(tuple('B'), LabelsType()), 'label_length': NeuralType(tuple('B'), LengthsType()),}

# new
{
    'label': NeuralType(tuple('B'), LabelsType()),
    'label_length': NeuralType(tuple('B'), LengthsType()),
}

Changelog

  • Introduce pre-commit, update Black version.

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you add or update any necessary documentation?
  • Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
    • Reviewer: Does the PR have correct import guards for all optional libraries?

PR Type:

  • New Feature
  • Bugfix
  • Documentation

@SeanNaren SeanNaren force-pushed the ci/pre-commit branch 2 times, most recently from 40e2c68 to 81f2d2e Compare September 21, 2022 10:42
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Copy link
Collaborator Author

I'm seeing quite a lot of code changes from Black, might need to play with the formatter to get it to replicate the current setup.

SeanNaren and others added 6 commits September 21, 2022 14:57
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
Signed-off-by: SeanNaren <snarenthiran@nvidia.com>
@SeanNaren
Copy link
Collaborator Author

Since we were using a really old version of Black, I was running into this: https://stackoverflow.com/questions/71673404/importerror-cannot-import-name-unicodefun-from-click

All suggestions say to upgrade Black, upgrading black also updated the formatting for many files. I think on some quick inspections the suggestions are fine (imo) but @ericharper @titu1994 let me know if it's reasonable.

I think eventually we would drop the style check from jenkins to reduce time.

@SeanNaren SeanNaren self-assigned this Sep 21, 2022
@SeanNaren SeanNaren marked this pull request as ready for review September 21, 2022 20:50
@SeanNaren SeanNaren marked this pull request as draft September 21, 2022 20:56
@SeanNaren
Copy link
Collaborator Author

Going to wait till 1.12 is released, so that we can pull this in without causing cherry-picking issues.

@SeanNaren SeanNaren mentioned this pull request Sep 22, 2022
8 tasks
@SeanNaren SeanNaren closed this Sep 28, 2022
@SeanNaren SeanNaren deleted the ci/pre-commit branch September 28, 2022 09:24
@SeanNaren
Copy link
Collaborator Author

Done in #5027

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