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

Ruff #250

Merged
merged 7 commits into from
Feb 24, 2023
Merged

Ruff #250

merged 7 commits into from
Feb 24, 2023

Conversation

janosh
Copy link
Member

@janosh janosh commented Feb 21, 2023

Closes #249.

Just to give an idea, this is what adding ruff linter would look like.

@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Merging #250 (8f60702) into main (970430e) will decrease coverage by 0.09%.
The diff coverage is 54.54%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #250      +/-   ##
==========================================
- Coverage   75.39%   75.31%   -0.09%     
==========================================
  Files          55       55              
  Lines        4723     4707      -16     
  Branches      692      682      -10     
==========================================
- Hits         3561     3545      -16     
- Misses        975      982       +7     
+ Partials      187      180       -7     
Impacted Files Coverage Δ
src/atomate2/amset/schemas.py 0.00% <0.00%> (ø)
src/atomate2/cli/dev.py 0.00% <0.00%> (ø)
src/atomate2/common/analysis/defects/schemas.py 91.66% <ø> (ø)
src/atomate2/common/jobs.py 50.00% <0.00%> (-31.82%) ⬇️
src/atomate2/common/schemas/elastic.py 88.54% <ø> (ø)
src/atomate2/common/schemas/symmetry.py 90.62% <ø> (ø)
src/atomate2/utils/file_client.py 43.25% <0.00%> (ø)
src/atomate2/vasp/flows/defect.py 94.87% <ø> (ø)
src/atomate2/vasp/flows/phonons.py 94.38% <ø> (ø)
src/atomate2/vasp/run.py 44.26% <0.00%> (ø)
... and 14 more

@utf
Copy link
Member

utf commented Feb 23, 2023

This looks great, but could you fix the failing linting?

@janosh
Copy link
Member Author

janosh commented Feb 23, 2023

Definitely. Just wanted to hear your feedback first.

@utf
Copy link
Member

utf commented Feb 23, 2023

Awesome thank you. Ruff looks excellent! I'll have to switch to it on all my projects

@janosh
Copy link
Member Author

janosh commented Feb 23, 2023

Awesome thank you. Ruff looks excellent! I'll have to switch to it on all my projects

That's what I did. 😄 And certainly didn't regret it. The speed is incredible as is the number of linting rules it provides!

- repo: https://github.com/pycqa/isort
rev: 5.12.0
hooks:
- id: isort
- repo: https://github.com/pycqa/flake8
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could prob replace flake8 here with ruff too. I know ruff implements these plugins since I was using those as well.

    - pyproject-flake8==6.0.0
    - flake8-bugbear==22.12.6
    - flake8-typing-imports==1.14.0
    - flake8-docstrings==1.6.0

But haven't checked these two yet:

    - flake8-rst-docstrings==0.3.0
    - flake8-rst==0.8.0

@janosh
Copy link
Member Author

janosh commented Feb 23, 2023

What should be ruff's max line length? Using the default 88 causes E501 errors. But a number different from black's 88 can cause linter fights since ruff also formats code. So if we change ruff's, we should also change black's.

E501s
src/atomate2/common/analysis/defects/schemas.py:46:89: E501 Line too long (109 > 88 characters)
src/atomate2/common/analysis/defects/schemas.py:51:89: E501 Line too long (109 > 88 characters)
src/atomate2/common/analysis/defects/schemas.py:56:89: E501 Line too long (103 > 88 characters)
src/atomate2/common/analysis/defects/schemas.py:61:89: E501 Line too long (103 > 88 characters)
src/atomate2/common/analysis/defects/schemas.py:66:89: E501 Line too long (112 > 88 characters)
src/atomate2/common/analysis/defects/schemas.py:71:89: E501 Line too long (112 > 88 characters)
src/atomate2/common/files.py:122:9: PLW2901 Outer for loop variable `file` overwritten by inner assignment target
src/atomate2/common/schemas/cclib.py:49:89: E501 Line too long (94 > 88 characters)
src/atomate2/common/schemas/cclib.py:118:89: E501 Line too long (90 > 88 characters)
src/atomate2/common/schemas/elastic.py:47:89: E501 Line too long (115 > 88 characters)
src/atomate2/common/schemas/elastic.py:51:89: E501 Line too long (116 > 88 characters)
src/atomate2/common/schemas/elastic.py:55:89: E501 Line too long (118 > 88 characters)
src/atomate2/common/schemas/elastic.py:74:89: E501 Line too long (102 > 88 characters)
src/atomate2/common/schemas/symmetry.py:59:89: E501 Line too long (94 > 88 characters)
src/atomate2/vasp/flows/phonons.py:199:89: E501 Line too long (92 > 88 characters)
src/atomate2/vasp/jobs/elastic.py:144:89: E501 Line too long (100 > 88 characters)
src/atomate2/vasp/schemas/calc_types/_generate.py:68:89: E501 Line too long (104 > 88 characters)
src/atomate2/vasp/schemas/calc_types/enums.py:380:89: E501 Line too long (89 > 88 characters)
src/atomate2/vasp/schemas/calc_types/enums.py:524:89: E501 Line too long (91 > 88 characters)
src/atomate2/vasp/schemas/calc_types/enums.py:752:89: E501 Line too long (93 > 88 characters)
src/atomate2/vasp/schemas/calc_types/enums.py:764:89: E501 Line too long (89 > 88 characters)
src/atomate2/vasp/schemas/calc_types/enums.py:776:89: E501 Line too long (91 > 88 characters)
src/atomate2/vasp/schemas/calc_types/enums.py:788:89: E501 Line too long (89 > 88 characters)
src/atomate2/vasp/schemas/calc_types/enums.py:800:89: E501 Line too long (89 > 88 characters)
src/atomate2/vasp/schemas/calc_types/enums.py:812:89: E501 Line too long (91 > 88 characters)
src/atomate2/vasp/schemas/calc_types/enums.py:824:89: E501 Line too long (89 > 88 characters)
src/atomate2/vasp/schemas/defect.py:32:89: E501 Line too long (91 > 88 characters)
src/atomate2/vasp/schemas/phonons.py:63:89: E501 Line too long (99 > 88 characters)
src/atomate2/vasp/schemas/phonons.py:161:89: E501 Line too long (100 > 88 characters)
src/atomate2/vasp/schemas/phonons.py:450:89: E501 Line too long (103 > 88 characters)
src/atomate2/vasp/schemas/task.py:391:89: E501 Line too long (90 > 88 characters)
src/atomate2/vasp/sets/base.py:836:89: E501 Line too long (103 > 88 characters)
src/atomate2/vasp/sets/base.py:837:89: E501 Line too long (104 > 88 characters)
src/atomate2/vasp/sets/base.py:844:89: E501 Line too long (103 > 88 characters)
src/atomate2/vasp/sets/base.py:845:89: E501 Line too long (104 > 88 characters)
tests/vasp/flows/test_core.py:227:89: E501 Line too long (89 > 88 characters)
tests/vasp/flows/test_core.py:277:89: E501 Line too long (89 > 88 characters)
tests/vasp/flows/test_elph.py:11:89: E501 Line too long (90 > 88 characters)
tests/vasp/flows/test_elph.py:16:89: E501 Line too long (108 > 88 characters)
tests/vasp/flows/test_phonon.py:813:89: E501 Line too long (98 > 88 characters)
tests/vasp/flows/test_phonon.py:817:89: E501 Line too long (95 > 88 characters)
tests/vasp/flows/test_phonon.py:981:89: E501 Line too long (92 > 88 characters)
tests/vasp/schemas/conftest.py:6:89: E501 Line too long (89 > 88 characters)
tests/vasp/schemas/test_defect.py:4:89: E501 Line too long (99 > 88 characters)

@utf
Copy link
Member

utf commented Feb 23, 2023

So I think black has the behaviour that 88 is the ideal length but if it can't make the line any shorter then it's okay to go beyond that.

I think there was an option in flake-bugbear to support this behaviour: PyCQA/flake8-bugbear#54

I'm not sure if that is also available in ruff?

@janosh
Copy link
Member Author

janosh commented Feb 23, 2023

IIUC astral-sh/ruff#389 (comment) suggests just disabling E501. Charlie decided to not support B950 for now.

@janosh
Copy link
Member Author

janosh commented Feb 23, 2023

I went ahead and fixed the existing E501s. All but one case were easily refactored to avoid long lines and the last one I added noqa: E501.

refactor caused     raise FileNotFoundError(f"Could not find {base_name} or {base_name}.gz file.")
FileNotFoundError: Could not find INCAR or INCAR.gz file.
@utf
Copy link
Member

utf commented Feb 24, 2023

Thanks!

@utf utf merged commit f22a535 into materialsproject:main Feb 24, 2023
@janosh janosh deleted the ruff branch February 24, 2023 14:55
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.

Migrate linting to Ruff
2 participants