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

Add ruff linter to pre-commit #5275

Merged
merged 6 commits into from
Dec 1, 2022

Conversation

Czaki
Copy link
Collaborator

@Czaki Czaki commented Oct 27, 2022

Description

Add ruff linter to pre-commit. Ruff is Rust written linter for python as a much faster replacement for flake8. Currently is not ready for total replacement, but maybe a nice addition as do not impact significantly on the pre-commit time run.

Also, this PR contains multiple fixes in napari files. Mostly add missed __all__ in __init__.py files.

Type of change

  • Refactor
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

References

How has this been tested?

  • example: the test suite for my feature covers cases x, y, and z
  • example: all tests pass with my change
  • example: I check if my changes works with both PySide and PyQt backends
    as there are small differences between the two Qt bindings.

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@github-actions github-actions bot added qt Relates to qt tests Something related to our tests labels Oct 27, 2022
@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #5275 (d9178af) into main (d33a6cd) will increase coverage by 0.06%.
The diff coverage is 92.00%.

❗ Current head d9178af differs from pull request most recent head c904a3f. Consider uploading reports for the commit c904a3f to get more accurate results

@@            Coverage Diff             @@
##             main    #5275      +/-   ##
==========================================
+ Coverage   89.10%   89.16%   +0.06%     
==========================================
  Files         584      584              
  Lines       49503    49534      +31     
==========================================
+ Hits        44110    44168      +58     
+ Misses       5393     5366      -27     
Impacted Files Coverage Δ
napari/components/experimental/remote/__init__.py 0.00% <0.00%> (ø)
napari/utils/events/__init__.py 100.00% <ø> (ø)
napari/utils/settings/__init__.py 0.00% <0.00%> (ø)
napari/_qt/__init__.py 56.66% <100.00%> (+8.39%) ⬆️
napari/_qt/layer_controls/__init__.py 100.00% <100.00%> (ø)
napari/_tests/test_examples.py 37.83% <100.00%> (+21.62%) ⬆️
napari/_vispy/__init__.py 100.00% <100.00%> (ø)
...omponents/experimental/chunk/_commands/__init__.py 100.00% <100.00%> (ø)
napari/components/experimental/monitor/__init__.py 100.00% <100.00%> (ø)
napari/components/overlays/__init__.py 100.00% <100.00%> (ø)
... and 23 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

examples/3d_kymograph_.py Outdated Show resolved Hide resolved
Carreau
Carreau previously requested changes Nov 1, 2022
Copy link
Contributor

@Carreau Carreau left a comment

Choose a reason for hiding this comment

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

Only one comment about ImportError -> ModuleNotFoundError.

The bare except is was even worse, even ImportError is an improvement.

@Czaki Czaki force-pushed the feature/add_ruff_linter_to_pre_commit branch from 4925ef2 to 5d7eb37 Compare November 23, 2022 10:37
@Czaki Czaki requested a review from Carreau November 27, 2022 12:28
@brisvag
Copy link
Contributor

brisvag commented Nov 28, 2022

I'm not convinced about the examples changes... Examples are supposed to be a quick way to get introduced to stuff, and I find it very distracting to see all those gibberish comments. I know what they mean, but anyone sho doesn't maintain a project probably would just be confused.

I'd say either we fix the examples so they obey the rules, or we skip examples.

@Czaki
Copy link
Collaborator Author

Czaki commented Nov 30, 2022

@Carreau You request changes, and it is blocking this PR. I think that I have addressed your worries but maybe not all?

I'm not convinced about the examples changes... Examples are supposed to be a quick way to get introduced to stuff, and I find it very distracting to see all those gibberish comments.

But if you open this code in any Python IDE same lines without these comments will be highlighted as errors.

@Czaki
Copy link
Collaborator Author

Czaki commented Nov 30, 2022

@brisvag I have workaround noqa clauses

examples/dynamic-projections-dask.py Show resolved Hide resolved
@@ -24,8 +24,8 @@
# important: if this is not set, the entire ~4GB array will be created!
os.environ.setdefault('NAPARI_OCTREE', '1')
Copy link
Contributor

Choose a reason for hiding this comment

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

does this have to be before import of napari? Otherwise, we can move it below to avoid the import lint errors.

Copy link
Collaborator Author

@Czaki Czaki Nov 30, 2022

Choose a reason for hiding this comment

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

Yes. It needs to be before. Currently, the octree is resolved on startup.

It is the weak point of napari but out of this PR scope.

@Carreau
Copy link
Contributor

Carreau commented Dec 1, 2022

@Carreau You request changes, and it is blocking this PR. I think that I have addressed your worries but maybe not all?

Feel free to dismiss reviews – or al least you should be able to dismiss them. I was not requesting changes as a way to block, but just suggestions.

@Czaki
Copy link
Collaborator Author

Czaki commented Dec 1, 2022

Feel free to dismiss reviews – or al least you should be able to dismiss them. I was not requesting changes as a way to block, but just suggestions.

How I could do this?

@brisvag
Copy link
Contributor

brisvag commented Dec 1, 2022

image

@Czaki
Copy link
Collaborator Author

Czaki commented Dec 1, 2022

If I had known this earlier...

Thanks @brisvag and @Carreau

@brisvag
Copy link
Contributor

brisvag commented Dec 1, 2022

If I had known this earlier...

Oh no, what have I done! I created a monster 😨

@Czaki Czaki merged commit 5ca30ea into napari:main Dec 1, 2022
@Czaki Czaki deleted the feature/add_ruff_linter_to_pre_commit branch December 1, 2022 16:27
@Czaki Czaki added the maintenance PR with maintance changes, label Dec 5, 2022
aganders3 pushed a commit to aganders3/napari that referenced this pull request Dec 20, 2022
melissawm pushed a commit to melissawm/napari that referenced this pull request Jan 11, 2023
@Czaki Czaki mentioned this pull request Jun 7, 2023
@Czaki Czaki added this to the 0.4.18 milestone Jun 13, 2023
@Czaki Czaki added the triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process label Jun 13, 2023
Czaki added a commit that referenced this pull request Jun 16, 2023
Czaki added a commit that referenced this pull request Jun 17, 2023
Czaki added a commit that referenced this pull request Jun 18, 2023
Czaki added a commit that referenced this pull request Jun 19, 2023
Czaki added a commit that referenced this pull request Jun 21, 2023
Czaki added a commit that referenced this pull request Jun 21, 2023
Czaki added a commit that referenced this pull request Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance PR with maintance changes, qt Relates to qt tests Something related to our tests triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants