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

Apply isort (forcing single lines, not sorting by type) via ruff #4662

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

peterjc
Copy link
Member

@peterjc peterjc commented Mar 14, 2024

Done using ruff v0.3.2 with this command:

    $ ruff check --fix --select=I \
      --config=lint.isort.force-single-line=true \
      --config=lint.isort.order-by-type=false \
      BioSQL/ Bio/ Tests/ Scripts/ Doc/ setup.py

This does also do some blank line changing before/after imports, and I removed a couple of redundant comments about standard library vs local imports.

[initially] This focused on modules I look after, but ulitmately I'd like to apply something like this globally. [Update: Done later]

  • I hereby agree to dual licence this and any previous contributions under both
    the Biopython License Agreement AND the BSD 3-Clause License.

  • I have read the CONTRIBUTING.rst file, have run pre-commit
    locally, and understand that continuous integration checks will be used to
    confirm the Biopython unit tests and style checks pass with these changes.

  • I have added my name to the alphabetical contributors listings in the files
    NEWS.rst and CONTRIB.rst as part of this pull request, am listed
    already, or do not wish to be listed. (This acknowledgement is optional.)

Cross reference #2828

@JoaoRodrigues
Copy link
Member

JoaoRodrigues commented Mar 14, 2024 via email

@JoaoRodrigues
Copy link
Member

JoaoRodrigues commented Mar 14, 2024 via email

@peterjc
Copy link
Member Author

peterjc commented Mar 14, 2024

I think this initial set of files suffices to show the feel of the changes, dropping brackets for multiple entry imports etc.

If there are no objections, I would be happy to expand the pull request to apply this to the whole code base - and add this import sorting to the automated checks.

@peterjc
Copy link
Member Author

peterjc commented Mar 14, 2024

Expanded to apply import sorting to the entire codebase as suggested, now a monster pull request!

@JoaoRodrigues
Copy link
Member

👹 👍 👍

@JoaoRodrigues
Copy link
Member

That's weird. The mmtf tests are failing only on MacOS and Windows because the dependency is missing, but going fine on Linux. We should maybe add a try/except to that import mmtf line? I wonder if changing the order of the imports triggered some sort of import caching in unittest, since it used to first import the Bio package that tests for mmtf and issues a warning.

@peterjc
Copy link
Member Author

peterjc commented Mar 15, 2024

Very odd, it worked locally but I do have mmtf installed - and on the other CI platforms. But as you say, macOS:

...
test_kNN ... ok
test_mmtf ... loading tests failed:
Failed to import test module: test_mmtf
Traceback (most recent call last):
  File "/Users/runner/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/unittest/loader.py", line 154, in loadTestsFromName
    module = __import__(module_name)
  File "/Users/runner/work/biopython/biopython/Tests/test_mmtf.py", line 16, in <module>
    import mmtf
ModuleNotFoundError: No module named 'mmtf'

test_mmtf_online ... skipping. Error: Install mmtf to use Bio.PDB.mmtf (e.g. pip install mmtf-python)

test_motifs ... ok
...

Note https://github.com/biopython/biopython/blob/master/Tests/test_mmtf.py just has import mmtf while https://github.com/biopython/biopython/blob/master/Tests/test_mmtf_online.py has from Bio.PDB.mmtf import MMTFParser which in turn calls https://github.com/biopython/biopython/blob/master/Bio/PDB/mmtf/__init__.py which has the try/except:

try:
    from mmtf import fetch, parse
except ImportError:
    from Bio import MissingPythonDependencyError

    raise MissingPythonDependencyError(
        "Install mmtf to use Bio.PDB.mmtf (e.g. pip install mmtf-python)"
    ) from None

So yes, for consistency we can add the try/except to test_mmtf.py

@peterjc
Copy link
Member Author

peterjc commented Mar 15, 2024

I understand the mmtf failure now. After sorting it does:

import mmtf  # may fail
...
from Bio.PDB.mmtf import MMTFParser, MMTFIO

Presorting we did this:

from Bio.PDB.mmtf import MMTFParser, MMTFIO  # catches failure 
...
import mmtf

@JoaoRodrigues
Copy link
Member

That's what it is... thanks for figuring that out!

@peterjc
Copy link
Member Author

peterjc commented Mar 15, 2024

I don't expect all the automatically assigned reviewers to review this - but would like more eyes on this before merging.

Note this PR would close #2828 by adopting the ruff implementation of an isort style.

@bzoracler
Copy link
Contributor

bzoracler commented Mar 15, 2024

As these are large formatting commits affecting hundreds of files, it might be helpful to skip these commits (along with those in #4655) for ease of viewing in git blame?

https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

@peterjc
Copy link
Member Author

peterjc commented Mar 16, 2024

@bzoracler good idea. For similar reasons I used something like black@example.org for the bulk style changes a while back.

@peterjc
Copy link
Member Author

peterjc commented Mar 28, 2024

Rebased to deal with the merge conflict in the configuration, and compressed the isort commits into one with a ruff@example.org dummy author.

Assuming we merge this, I think a followup PR adding a .git-blame-ignore-revs file for this and previous bulk edits like black formatting and some of style changes would make sense.

@peterjc
Copy link
Member Author

peterjc commented Apr 2, 2024

The Python 3.10 failure was #4640 (since fixed). I'll rebase again for the merge conflicts...

Ruff isort fixer and others added 3 commits April 11, 2024 16:50
Using ruff v0.3.2 with this command:

$ ruff check --fix --select=I \
  --config=lint.isort.force-single-line=true \
  --config=lint.isort.order-by-type=false \
  BioSQL/ Bio/ Tests/ Scripts/ Doc/ setup.py

This does also do some blank line changing before/after imports,
and I removed a couple of redundant comments about standard library
vs local imports.
All the other usage was using from None
@peterjc
Copy link
Member Author

peterjc commented Apr 11, 2024

Rebased for trivial version updates in the pre-commit config; and some whitespace cleanup spotted while comparing branches.

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.

None yet

3 participants