Skip to content

Pycln QA tool as a pre-commit hook - #2001

Merged
willmcgugan merged 8 commits into
Textualize:masterfrom
hadialqattan:pycln
Mar 15, 2022
Merged

willmcgugan merged 8 commits into
Textualize:masterfrom
hadialqattan:pycln

Conversation

@hadialqattan

Copy link
Copy Markdown
Contributor

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate. (not needed for this PR)
  • I've added tests for new code. (not needed for this PR)
  • I accept that @willmcgugan may be pedantic in the code review.

Description

I saw pycln (which is a formatter for finding and removing unused import statements) a good addition to the current pre-commit hooks which I believe would ensure a high code quality standard besides black and isort.

BTW, I'm the author of pycln 😄.

Anyway, please feel free to ask any questions and/or reject/close this PR.

+ I would like to thank you for inventing this incredible library!

@willmcgugan

Copy link
Copy Markdown
Member

It's a good idea! Do you have examples of other notable projects using pycln

@codecov-commenter

codecov-commenter commented Feb 26, 2022 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.48%. Comparing base (91e0146) to head (5a93a6e).
⚠️ Report is 1542 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2001      +/-   ##
==========================================
- Coverage   99.49%   99.48%   -0.02%     
==========================================
  Files          73       72       -1     
  Lines        7361     7335      -26     
==========================================
- Hits         7324     7297      -27     
- Misses         37       38       +1     
Flag Coverage Δ
unittests 99.48% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hadialqattan

hadialqattan commented Feb 26, 2022 •

Copy link
Copy Markdown
Contributor Author

It's a good idea! Do you have examples of other notable projects using pycln

For now, fossasia's open event server is the most notable project that I can name. (they're using pycln from a few months ago).

Also, someone ran pycln against poetry in this PR then one of the poetry maintainers/active contributors said:

A second PR adding pycln to the pre-commit config would be welcome, I think.

Consensus seems to be that the new Union syntax is harmless and nicer to write. Let's go ahead with this.

Originally posted by @neersighted in python-poetry/poetry#5088 (comment)

therefore I've created a PR in poetry to add pycln to their pre-commit hooks and it's about to be merged.


In fact, in the last few days I offered Pycln to several mainstream projects so maybe in the next few weeks we will see more notable projects using it.

@henryiii

henryiii commented Mar 3, 2022

Copy link
Copy Markdown
Contributor

I've included pycln in the Scikit-HEP developer pages for a while, so projects that follow that sometimes use it, like https://github.com/pybind/pybind11, https://github.com/pyodide/pyodide, https://github.com/pypa/cibuildwheel, and of course a few Scikit-HEP libraries. My favorite is likely Pyodide's usage, as it's a great manual hook - running it by default can be a little overkill (a typo when using a library can cause that library to be removed, for example), but it's really handy during refactors, like from __future__ import annotations additions, etc.

@hadialqattan

hadialqattan commented Mar 3, 2022 •

Copy link
Copy Markdown
Contributor Author

I've included pycln in the Scikit-HEP developer pages for a while, so projects that follow that sometimes use it, like https://github.com/pybind/pybind11, https://github.com/pyodide/pyodide, https://github.com/pypa/cibuildwheel, and of course a few Scikit-HEP libraries. My favorite is likely Pyodide's usage, as it's a great manual hook - running it by default can be a little overkill (a typo when using a library can cause that library to be removed, for example), but it's really handy during refactors, like from __future__ import annotations additions, etc.

That's awesome, in fact, I already know that those projects use Pycln, but I forgot about them for some reason...Anyway thanks a lot @henryiii for supporting Pycln and using it.

@hadialqattan

Copy link
Copy Markdown
Contributor Author

It's a good idea! Do you have examples of other notable projects using pycln

I have included a used by section in the README file containing a list of notable projects/organizations that use Pycln.

@willmcgugan

Copy link
Copy Markdown
Member

@hadialqattan Ok, let's give it a go :)

I created a merge conflict for you. Would you mind fixing that and I'll merge?

@hadialqattan

Copy link
Copy Markdown
Contributor Author

@hadialqattan Ok, let's give it a go :)

I created a merge conflict for you. Would you mind fixing that and I'll merge?

I've fixed it, but please let me double check my work again.

@hadialqattan

Copy link
Copy Markdown
Contributor Author

Now we are good to go 👍🏻

@hadialqattan

hadialqattan commented Mar 11, 2022 •

Copy link
Copy Markdown
Contributor Author

@willmcgugan
btw, sorry for bothering you by my recent commits which triggers workflows approval request over and over again.

@willmcgugan

Copy link
Copy Markdown
Member

THanks.

@willmcgugan
willmcgugan merged commit e0c9d0d into Textualize:master Mar 15, 2022
@hadialqattan
hadialqattan deleted the pycln branch March 16, 2022 04:32
@willmcgugan

Copy link
Copy Markdown
Member

I was worried about this. The PR broke the IPython extension. Is there any way to tell it not to touch a given import?

@hadialqattan

Copy link
Copy Markdown
Contributor Author

I was worried about this. The PR broke the IPython extension. Is there any way to tell it not to touch a given import?

Yes, of course, please take a look at the Pycln skips section.

btw, @henryiii has already fixed it in #2109.

@hadialqattan

hadialqattan commented Mar 25, 2022 •

Copy link
Copy Markdown
Contributor Author

@willmcgugan In case you wanna know why Pycln didn't treat it as used by default, please have a look at the following sources:

btw, not using the --all flag seems safer but I encourage using it because the false-positive cases usually occur much more than the false-negative ones. Also, it's easier to spot and correct the false-negative ones.

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.

4 participants