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.

Sign up for GitHub

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

Jump to bottom

MRG: Use ruff-format instead of Black #12261

Merged
merged 20 commits into from
Dec 5, 2023
Merged

MRG: Use ruff-format instead of Black #12261

merged 20 commits into from
Dec 5, 2023

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Dec 4, 2023 •
edited
Loading

Closes #12255

I also removed numerous unnecessary # noqa: D102's from __init__() methods. (Unnecessary there if the class itself has a docstring)

@hoechenberger hoechenberger mentioned this pull request Dec 4, 2023
Closed
@hoechenberger hoechenberger marked this pull request as ready for review December 4, 2023 13:28
@cbrnr
Copy link
Contributor

cbrnr commented Dec 4, 2023 •
edited
Loading

Style checks are still duplicated with "Tests / Style" (GHA) and "pre-commit.ci" checks. I think we can remove the "pre-commit.ci" check, since we disabled autofix. @larsoner?

cbrnr
cbrnr reviewed Dec 4, 2023
View reviewed changes
@mscheltienne
Copy link
Member

What is the benefit of ruff-format compared to black? Especially as black is available in many IDEs and ruff is not?

@cbrnr
Copy link
Contributor

cbrnr commented Dec 4, 2023 •
edited
Loading

@mscheltienne

@mscheltienne
Copy link
Member

mscheltienne commented Dec 4, 2023 •
edited
Loading

OK found the linked issue, speed sounds good, although I would also be inclined to give them some time to refine this new functionality.
@cbrnr thanks,I knew about that one and have it setup, not sure if it's integrated in pycharm, and definitely not in spyder.

EDIT: OK so just Spyder lagging behind, not a big issue.

@hoechenberger
Copy link
Member Author

What is the benefit of ruff-format compared to black? Especially as black is available in many IDEs and ruff is not?

It's faster and one tool less to run via pre-commit hooks, making for a smoother dev experience if these hooks are installed

@hoechenberger
Copy link
Member Author

hoechenberger commented Dec 4, 2023 •
edited
Loading

EDIT: OK so just Spyder lagging behind, not a big issue.

Spyder can use it via python-lsp-ruff in https://github.com/python-lsp/python-lsp-server

@larsoner
Copy link
Member

larsoner commented Dec 4, 2023

Style checks are still duplicated with "Tests / Style" (GHA) and "pre-commit.ci" checks. I think we can remove the "pre-commit.ci" check, since we disabled autofix. @larsoner?

I'd rather move more stuff to pre-commit.ci when possible actually. It tends to be faster than GHA and distributes load better to have it there. I think it makes sense (and we have been moving toward) to have pre-commit + pre-commit.ci take care of as much as possible, and leave the checks that are too slow for pre-commit (like twine check) to the remaining GHA style check

larsoner
larsoner reviewed Dec 4, 2023
View reviewed changes
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

+1 for merge from me, with or without looking into the " + ".*" stuff. Overall I think this improves things and we can manually fix the " + ".*" when we come across them (or wait for the tool to get better and fix these for us automatically!).

mne/beamformer/tests/test_rap_music.py
1
/ (sigma * np.sqrt(2 * np.pi))
* np.exp(-((times - mu) ** 2) / (2 * sigma**2))
1 / (sigma * np.sqrt(2 * np.pi)) * np.exp(-((times - mu) ** 2) / (2 * sigma**2))
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering why we had a ton of lines removed... I'm guessing it's mostly from this. I consider this a nice readability win

Copy link
Member Author

Choose a reason for hiding this comment

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

Same.

@hoechenberger
Copy link
Member Author

hoechenberger commented Dec 4, 2023 •
edited
Loading

@larsoner Currently, some GHA jobs like the one that runs pytest depend on the "style" job, which runs the pre-commit hooks. This is to avoid running pytest as long as there are still code style issues.

Can we make our GHA runs conditional on pre-commit.ci return codes?

@larsoner
Copy link
Member

larsoner commented Dec 4, 2023

Can we make our GHA runs conditional on pre-commit.ci return codes?

Possibly but someone would have to look into it. FYI Azure also has a style run that other runs depend on IIRC

@hoechenberger
Copy link
Member Author

But that would mean we run the pre-commit hooks twice (pre-commit.ci and GHA)? Unless we remove the job from GHA, businesses then the others cannot be (easily) conditional

What do you suggest we do?

@larsoner
Copy link
Member

larsoner commented Dec 4, 2023

I think the way things work right now are okay. There's always room for improvement but I think we have more important stuff to work on (diminishing returns tweaking CIs further)

drammock
drammock reviewed Dec 4, 2023
View reviewed changes
hoechenberger
hoechenberger commented Dec 5, 2023
View reviewed changes
tutorials/stats-source-space/60_cluster_rmANOVA_spatiotemporal.py
@@ -285,9 +285,7 @@ def stat_fun(*args):

inds_t, inds_v = [
(clusters[cluster_ind]) for ii, cluster_ind in enumerate(good_cluster_inds)
][
0
] # first cluster
Copy link
Member Author

Choose a reason for hiding this comment

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

@drammock here's an example where things are reversed and are looking better with ruff

hoechenberger and others added 11 commits December 5, 2023 09:54
@hoechenberger @drammock
Apply suggestions from code review
3027dc3 
Co-authored-by: Daniel McCloy <[email protected]>
@hoechenberger @drammock
Apply suggestions from code review
f647a0e 
Co-authored-by: Daniel McCloy <[email protected]>
@hoechenberger @drammock
Apply suggestions from code review
ab07566 
Co-authored-by: Daniel McCloy <[email protected]>
@hoechenberger @drammock
Apply suggestions from code review
8164953 
Co-authored-by: Daniel McCloy <[email protected]>
@hoechenberger @drammock
Apply suggestions from code review
834ec27 
Co-authored-by: Daniel McCloy <[email protected]>
@hoechenberger @drammock
Apply suggestions from code review
785d2a2 
Co-authored-by: Daniel McCloy <[email protected]>
@hoechenberger @drammock
Apply suggestions from code review
7c46976 
Co-authored-by: Daniel McCloy <[email protected]>
@hoechenberger
Run ruff format again
1574b26
@hoechenberger
Fix dipole tutorial
0930e45
@hoechenberger
Merge branch 'main' of https://github.com/mne-tools/mne-python into r…
3bdecee 
…uff-format
@hoechenberger
Better docstring as suggested by drammock
9ec2180
@hoechenberger hoechenberger changed the title Demonstration: Use ruff-format instead of Black MRG: Use ruff-format instead of Black Dec 5, 2023
@hoechenberger
Copy link
Member Author

I think this is good to go.

@hoechenberger
Copy link
Member Author

@mscheltienne Okay with you if we move forward here?

@mscheltienne
Copy link
Member

Sure!

@larsoner larsoner merged commit e7dd158 into mne-tools:main Dec 5, 2023
@larsoner
Copy link
Member

larsoner commented Dec 5, 2023

Okay let's give it a shot!

I'll add e7dd158 to .git-blame-revs-ignore in some follow-up PR

larsoner added a commit to hoechenberger/mne-python that referenced this pull request Dec 5, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into type-hints
59646ef 
* upstream/main:
  MRG: Use ruff-format instead of Black (mne-tools#12261)
  MAINT: Make selenium optional and use on CircleCI (mne-tools#12263)
  MAINT: Post-release deprecations (mne-tools#12265)
  MAINT: Replace `Path.parent.parent` with `Path.parents[N]` in tests (mne-tools#12257)
@hoechenberger hoechenberger deleted the ruff-format branch December 5, 2023 17:30
@mscheltienne mscheltienne mentioned this pull request Dec 20, 2023
Merged
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
@hoechenberger @larsoner
@drammock @snwnde
MRG: Use ruff-format instead of Black (mne-tools#12261)
7906f20 
Co-authored-by: Eric Larson <[email protected]>
Co-authored-by: Daniel McCloy <[email protected]>
@bruAristimunha bruAristimunha mentioned this pull request Apr 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drammock drammock drammock left review comments

@larsoner larsoner larsoner left review comments

@cbrnr cbrnr cbrnr left review comments

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@dengemann dengemann Awaiting requested review from dengemann dengemann is a code owner

@adam2392 adam2392 Awaiting requested review from adam2392 adam2392 is a code owner

@mscheltienne mscheltienne Awaiting requested review from mscheltienne mscheltienne is a code owner

@alexrockhill alexrockhill Awaiting requested review from alexrockhill alexrockhill is a code owner

@jasmainak jasmainak Awaiting requested review from jasmainak jasmainak is a code owner

@britta-wstnr britta-wstnr Awaiting requested review from britta-wstnr britta-wstnr is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

We may be able to ditch Black in favor of just Ruff
5 participants
@hoechenberger @cbrnr @mscheltienne @larsoner @drammock