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

Use pre-commit to automatically format code with Black and imports with isort #345

Closed
CAM-Gerlach opened this issue May 2, 2022 · 4 comments · Fixed by #451
Closed

Use pre-commit to automatically format code with Black and imports with isort #345

CAM-Gerlach opened this issue May 2, 2022 · 4 comments · Fixed by #451
Assignees
Milestone

Comments

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented May 2, 2022

As suggested by @DaelonSuzuka in #344 ,

It would also help readability and auditability to standardize the formatting of these files.

Indeed, for a while now the QtPy source has followed an inconsistent smattering of different PEP 8 and non-PEP 8-conforming styles and practices, which make it harder to easily read and edit, and creates extra work for maintainers and contributors to determine and conform to a consistent code style, as well as extra diff noise and merge conflict potential. This also results in a lot of noisy comments in PR reviews for issues that are primary style-related. This is particularly true since much of QtPy's "code" is import statements for the various names from the different bindings, and given the limited documentation, its particularly important for the code, particularly the imports, to be easily readable for others.

To both improve import and code readability and consistency, and avoid contributors and maintainers having to spend time checking, implementing and enforcing code style, I suggest adding Black to format the code and isort to automatically organize the imports (including conforming to our own custom headings and ordering rules). Adding them via pre-commit, as we do for a number of our other repos, makes it easy for maintainers and contributors to run them automatically whenever they commit, or on-demand, without having to worry about installing, configuring and manually running tooling, and our CIs can use the same as a check. They can also run them individually, or via editor/IDE integration (including in Spyder), if they prefer.

I've already configured this in other projects, so its mostly a matter of dropping in the existing config plus any QtPy-specific config customization. Since we only have one open PR, #344 , aside from a several-year old, stale and heavily conflicting one that would need to be redone anyway, this should be minimally disruptive. I can add the contributor documentation for running pre-commit from our other repos; it basically just boils down to installing pre-commit (if its not already a dev dependency) and running pre-commit install, and everything else (including installing, updating and running hooks) requires zero additional action on the part of the user.

@dalthviz
Copy link
Member

Some suggestions on this topic: #433 (comment)

@Czaki
Copy link
Contributor

Czaki commented Aug 25, 2023

one of latest changes is that it is better to use https://github.com/psf/black-pre-commit-mirror instead of https://github.com/psf/black in pre-commit configuration as first uses pre-compiled wheels and is about 4 times faster.

@Czaki
Copy link
Contributor

Czaki commented Aug 25, 2023

@dalthviz I see that you have .pre-commit-config.yaml with isort and black. What is the real task? Lack of CI setup?
To big configuration (tom any checks)?

@dalthviz
Copy link
Member

The current pre-commit file was added at some point by mistake (when some initial attempt to add pre-commit was done) and seems like is failing (see comment note at #450 (comment)). So the idea would be to:

  • Fix issues with the current pre-commit config definition
  • Run it one first time to make the whole source code formatted
  • Add some CI check
  • Add an entry mentioning the use of pre-commit under the CONTRIBUTING guide

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

Successfully merging a pull request may close this issue.

3 participants