Skip to content
This repository has been archived by the owner on Dec 20, 2024. It is now read-only.

CI: replace flake8 by ruff check #139

Merged
merged 10 commits into from
Mar 27, 2024
Merged

Conversation

esavary
Copy link
Member

@esavary esavary commented Mar 21, 2024

  • Replace flake8 CI job with a ruff CI job.

@esavary esavary changed the title CI: replace flake8 by ruff check CI: replace flake8 by ruff check - WIP Mar 21, 2024
@esavary
Copy link
Member Author

esavary commented Mar 22, 2024

In pyproject.toml, I'm wondering how to choose the error codes for the section [tool.ruff.lint]. Should we use the same as fmriprep or mriqc, or create specific rules for eddymotion? What are your thoughts on this @effigies @oesteban ?

@oesteban
Copy link
Member

Except for src/eddymotion/estimator.py:43:9: C901 fit is too complex (17 > 10), I don't think we want to have exceptions there. Copying from fmriprep/mriqc does not seem useful at all -- you want to be very explicit about what you add as an exception.

And maybe we should look into that C901 and try to pacify ruff by improving the code, as opposed to adding one exception.

@esavary
Copy link
Member Author

esavary commented Mar 22, 2024

Except for src/eddymotion/estimator.py:43:9: C901 fit is too complex (17 > 10), I don't think we want to have exceptions there. Copying from fmriprep/mriqc does not seem useful at all -- you want to be very explicit about what you add as an exception.

And maybe we should look into that C901 and try to pacify ruff by improving the code, as opposed to adding one exception.

More explicitly, are you suggesting adding B028 and C408 to the list of errors to ignore and modifying estimator.py to address C901?

@oesteban
Copy link
Member

Except for src/eddymotion/estimator.py:43:9: C901 fit is too complex (17 > 10), I don't think we want to have exceptions there. Copying from fmriprep/mriqc does not seem useful at all -- you want to be very explicit about what you add as an exception.
And maybe we should look into that C901 and try to pacify ruff by improving the code, as opposed to adding one exception.

More explicitly, are you suggesting adding B028 and C408 to the list of errors to ignore and modifying estimator.py to address C901?

I think we should not have exceptions. We may add C901 temporarily so that this PR passes but open an issue to remind us of looking into it at a later time.

@esavary
Copy link
Member Author

esavary commented Mar 22, 2024

Except for src/eddymotion/estimator.py:43:9: C901 fit is too complex (17 > 10), I don't think we want to have exceptions there. Copying from fmriprep/mriqc does not seem useful at all -- you want to be very explicit about what you add as an exception.
And maybe we should look into that C901 and try to pacify ruff by improving the code, as opposed to adding one exception.

More explicitly, are you suggesting adding B028 and C408 to the list of errors to ignore and modifying estimator.py to address C901?

I think we should not have exceptions. We may add C901 temporarily so that this PR passes but open an issue to remind us of looking into it at a later time.

Can you provide more details? If I leave out B028 and C408 from the exceptions, the PR won't pass. Right now, I've used error codes from the old Flake8 configuration for the extend and ignore lists in [tool.ruff.lint] in the pyproject.toml, but I'm not sure if this is the most effective approach.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
esavary and others added 2 commits March 22, 2024 15:03
Co-authored-by: Oscar Esteban <[email protected]>
Co-authored-by: Oscar Esteban <[email protected]>
@oesteban
Copy link
Member

Okay, we have this at the moment:

src/eddymotion/data/dmri.py:254:13: B028 No explicit `stacklevel` keyword argument found
src/eddymotion/model/base.py:93:18: C408 Unnecessary `tuple` call (rewrite as a literal)
src/eddymotion/viz.py:346:24: C408 Unnecessary `dict` call (rewrite as a literal)
src/eddymotion/viz.py:398:22: C408 Unnecessary `dict` call (rewrite as a literal)

These are trivial changes we can (and should) address.

@esavary
Copy link
Member Author

esavary commented Mar 22, 2024

Okay, we have this at the moment:

src/eddymotion/data/dmri.py:254:13: B028 No explicit `stacklevel` keyword argument found
src/eddymotion/model/base.py:93:18: C408 Unnecessary `tuple` call (rewrite as a literal)
src/eddymotion/viz.py:346:24: C408 Unnecessary `dict` call (rewrite as a literal)
src/eddymotion/viz.py:398:22: C408 Unnecessary `dict` call (rewrite as a literal)

These are trivial changes we can (and should) address.

Should this be addressed in this PR, or would it be more appropriate for a separate PR? (Considering the PR subject is transitioning from flake8 to ruff)

@oesteban
Copy link
Member

It's only four little changes -- I would stuff it in this one and reduce the noise overall

@esavary esavary requested review from effigies and oesteban March 22, 2024 15:22
@esavary esavary changed the title CI: replace flake8 by ruff check - WIP CI: replace flake8 by ruff check Mar 26, 2024
@esavary esavary mentioned this pull request Mar 26, 2024
Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

LGTM

@esavary esavary merged commit d23dcf0 into nipreps:main Mar 27, 2024
8 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants