[INFRA] Several style fixes (Flake8) for Python code in the repo #872
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remi-Gau
approved these changes
Sep 13, 2021
+1, that'd be more efficient
+1 for black :-) |
sappelhoff
reviewed
Sep 13, 2021
There was a problem hiding this comment.
@DimitriPapadopoulos could you please elaborate on what error you fixed, and what the situation was BEFORE versus AFTER the fix? Preferably with screenshots and/or lines of code? :-)
|
I seem to recall haven't fixed all flake8 issues, only those that I felt were the most relevant for readability. So why not add flake8 to CI, but perhaps disable a few errors - or not. I find black really too intrusive - besides there are forks such as oitnb for that very reason (for example accept |
|
BEFORE (111 issues) AFTER (60 issues) |
|
If you disregard |
DimitriPapadopoulos
force-pushed
the
flake8
branch
from
2c47af6
to
644d9f2
Compare
sappelhoff
reviewed
Sep 13, 2021
there we have our linter war 🎉 🙂 Maybe a simple flake8 checker will suffice for now then. Eventually, most of the code currently in this repo will be migrated to their own repos. |
|
Oh, there's no linter war here 😄 I can adapt to any style, as long as I find it readable. Oh wait! That's not the case for black, but it's not totally unreadable either, so I can live with it. Whatever you prefer. The real issue here is minimizing changes so as not to pollute the git log too much with stylistic issues. It's OK to run black on new projects, but it's too intrusive for existing projects. At least with flake8 you can disable errors such as |
|
yes, sounds good to me 👍 |
|
At least for EDIT: @DimitriPapadopoulos I just saw your comment about |
DimitriPapadopoulos
force-pushed
the
flake8
branch
from
644d9f2
to
f8bf13a
Compare
sappelhoff
changed the title
sappelhoff
approved these changes
Sep 15, 2021
|
I can run black on |
from my side yes, I like BLACK and believe it delivers the advantages it promises. But if we do that, we also need to add a CI check for black formatting --> and that might be ugly for new contributors (need to install and run an additional package; or need to download and install a pre-commit hook). So maybe overall I am 50 50 on this |
|
Then |
|
just a note that for some of our other projects we use pre-commit.ci which is a service that integrates with github and automatically runs whichever precommits you configure. this may help alleviate user configuration. but they will have to download the updated branch to continue. however, that service helps a lot for github based edits, which is often the case now for many suggestions. |
This is a real error, not just a style issue.
DimitriPapadopoulos
force-pushed
the
flake8
branch
from
f8bf13a
to
3bcda38
Compare
|
Thanks @satra for bringing this up, I remember @Remi-Gau thinking about that as well. I am tempted to just merge this PR, and if people care deeply about this - continue the discussion in a dedicated issue, before we make changes. The commits here contain one bugfix next to the style fixes, so they should not stay unmerged for so long :-) |
OK to merge and move black styling + pre-commit to another issue. |
|
Thanks @DimitriPapadopoulos |
bids-maintenance
added a commit
that referenced
this pull request
Sep 28, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first commit fixes an actual error.
The second commit is about style. I agree style is a question of taste, on the other hand using the standard style does help later maintenance of the code, by other programmers.