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

tools/gen-cpydiff.py: Fix formatting. #7168

Closed
wants to merge 1 commit into from

Conversation

iabdalkader
Copy link
Contributor

No description provided.

@stinos
Copy link
Contributor

stinos commented Apr 27, 2021

This file is already formatted by code-format.py I think, so not sure this fix is really worth it. Or is this something newer versions of black do, i.e. what lead you to these changes? If anything, imo it would be better to at once apply formatting more consistently: if I'm not mistaken PEP8 wants those comments to start with a capital and end with a dot. Which is also what most other files in the tools/ directory seem to do. But in any case it would be better if all comments in the file look the same. And you missed the comment at the top as well. Did you consider other files in this directory as well?

@stinos
Copy link
Contributor

stinos commented Apr 27, 2021

Likely this is the reason psf/black#1740. It's in the release 21.4b0 which gets used by the CI.
Please mention this in the commit message.

@dpgeorge : when merged the commit SHA should also be put into .git-ignore-revs

@dpgeorge dpgeorge added the tools label Apr 27, 2021
@dpgeorge
Copy link
Member

Thanks for this! Merged in 0f78c36 with improved commit message, and additional commit 30d9f77 for change to .git-blame-ignore-revs

@dpgeorge dpgeorge closed this Apr 27, 2021
@iabdalkader iabdalkader deleted the format branch April 27, 2021 14:51
@iabdalkader
Copy link
Contributor Author

what lead you to these changes

Hi sorry for not replying earlier, I sent another PR and it failed checks because of this file so I ran codeformat.py on it and sent this PR to fix it first, now both PRs are merged @stinos

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

Successfully merging this pull request may close these issues.

None yet

3 participants