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

[WIP] Introduce ruff code-format style #934

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

asoplata
Copy link
Collaborator

@asoplata asoplata commented Nov 8, 2024

This is a PR for both introducing stylistic formatting using the incredibly fast ruff package (see https://docs.astral.sh/ruff/ ), and replacing our flake8 linting also with the linting provided by ruff. This will result in very many lines of code changes, but should not affect functionality at all.

Some of us were recently discussing if we should adopt a particular style, especially since the code right now is inconsistent. I think this is an opportunity to introduce ruff into the dev pipeline, both as our formatter of choice, and as our linter of choice (replacing flake8). Pros of this approach:

  1. ruff is unbelievably fast, both at formatting (both detecting and applying/editing format changes in-place) and linting. Our codebase is relatively small, but IMHO it is effectively instantaneous when I try it on both of these tasks, even on a 5-year-old Intel i5 processor.
  2. I personally really really love the default styling of ruff's formatting, which I think is identical to black's by default (e.g. line length 88 chars, etc.). Currently in this PR, the only non-default format style change is my preference for single quotes instead of double. Note that the longer line length is in line (pun intended) with other users of ruff like Scipy (see https://scipy.github.io/devdocs/dev/contributor/pep8.html#pep8-and-scipy ). I personally prefer the C-style braces and emphasis on verticality, but whatever style we decide needs to be a group decision.
  3. ruff's formatting is very configurable. We can easily make changes to our preferred format style.
  4. I can't tell if flake8 will actually edit your code to match particular formatting (which ruff does). As far as I can tell, flake8 can only inform you of certain formatting findings (but will not edit your code). I could be wrong here. If flake8 cannot edit the formatting of our code, then we will need to install or setup another package to do that anyways (so it might as well be ruff).
  5. In many cases, ruff's linting can be set to automatically fix individual issues. I don't know if flake8 supports this. I personally prefer to make the fixes myself so I understand them, but it's convenient.
  6. ruff's linting is also extremely fast and configurable.
  7. ruff doesn't currently have 100% support of pycodestyle rules (see Implement remaining pycodestyle rules astral-sh/ruff#2402 ), but the vast majority are there, and it's definitely enough coverage to make it worth using.

Finally, I will note that after creating the huge-formatting-commit, I ran all the tests locally and they passed, and I also successfully rebuilt the docs locally just in case as well.

If we want, we can use this PR for discussing format changes we want to make, that way I can rebase them on this branch and refrain from polluting the master branch with changes until we're happy.

I will also use this PR to get ruff's linting up and running, both fixing issues that flake8 missed, and ensuring the CI runs of ruff's linting work.

@asoplata asoplata changed the title Introduce ruff formatting, and replace flake8 linting with ruff linting [WIP] Introduce ruff formatting, and replace flake8 linting with ruff linting Nov 11, 2024
@asoplata asoplata added this to the 0.5 milestone Nov 12, 2024
@ntolley
Copy link
Contributor

ntolley commented Nov 13, 2024

I like the plan! Can we discuss this after we're done with steering committee prep?

@asoplata
Copy link
Collaborator Author

I like the plan! Can we discuss this after we're done with steering committee prep?

Absolutely. We may even want to delay this until after v0.4, depending on if it introduces difficulties cleaning up the outstanding PRs.

@jasmainak
Copy link
Collaborator

@asoplata lot of points to think about :) I'm fine with using ruff as a linter ... I would start with configuring ruff such that the number of stylistic changes are limited (<100 lines of code). Having huge diffs is not a recipe for success in open source repos ;-) [even if straightforward]. Smaller diffs are likely to be approved faster with less controversy. Look around in other repos how they have transitioned (e.g., mne-python). The more we copy what is the standard in the Python ecosystem, the better it is!

@jasmainak
Copy link
Collaborator

Btw, flake8 is just the linter ... the style guideline is pep8. There is a tool called autopep8 which does the automatic fixing for you. I personally tend to prefer linting as opposed to automatic fixing since it makes me aware of the style guidelines so the code style is not an afterthought. The automatic tool does lower the barrier to entry for contributing. But it's hard to trust code that was written as a mess only to be fixed using an automatic tool. But I know people have their preferences ;-) I let you decide what's the best path forward as a community

@asoplata
Copy link
Collaborator Author

Thanks for your insight Mainak! Seeing how MNE does it will be an excellent reference point.

Regarding linting, as you're probably aware (and what my commits in this PR made more confusing), the linting of ruff check, without any arguments, does not automatically fix lint errors. When it comes to linting, I definitely agree with you that linted errors in general should be evaluated manually, to understand what is going on. I should remove the unnecessary --no-fix arguments I added, since they don't change anything and only serve to confuse. I don't think we should be including any automatic error fixing. Note that the only changes needed for ruff linting (as opposed to formatting) are in this commit, and are only a handful of lines c3da09a .

Regarding formatting, I definitely understand that Godzilla-sized, N x kloc PR diffs are not good practice, but I think that any instance of switching from just the pretty-loose pep8 constraints to significantly "tighter" and more opinionated styling is always going to result in many lines of code changes. MNE had to go through similar pains when Black (basically same as ruff) was introduced here mne-tools/mne-python@e81ec52 . I personally find Black-style tighter formatting to be significantly more readable than what is current in hnn-core. More importantly, I think styling being consistent at all also enhances the readability, and that goes double when we share 99% of our styling with similar packages like MNE. Unlike the linter, the ruff formatter does make fixes in-place, but I think this is fine since it definitely should not introduce errors. Having an automated formatter also means you don't have to spend any thought on formatting (unlike linting errors).

I experimented with running autopep8 --diff --recursive --exclude=__init__.py --max-line-length=88 hnn_core/* before and after the huge ruff-format changes, and the only changes autopep8 identified after ruff formatting were 3 invalid finds (specifically, autopep8 wanted to introduce this pep8-invalid change hhatto/autopep8#723 which is surely a bug).

I will definitely make a new issue about using pre-commit, and also any more recommendations we can share from MNE :)

@jasmainak
Copy link
Collaborator

Can you explain the difference between formatting and linting? How can we explain to new contributors how to use ruff? e.g., is there a VSCode plug in etc? Should be added to contributing guidelines.

I would split this PR into two.

The first one just introduces ruff with roughly the same issues as flake8. Use ignore rules. In this PR you would make sure that everything is in place for people to migrate to ruff. The contributing guide should be updated and make sure that any other open PRs that may be affected are either closed or merged. The diff of this PR should be relatively small. This way you avoid rebase issues.

In the next PR, remove the ignore rules and make ruff more strict. This PR should be turned around fast so that people working on new PRs start from the new codebase.

@asoplata
Copy link
Collaborator Author

I will separate the linting part of this PR into its own, since 1. the linting is a very small change, 2. formatting is a much, much bigger change, and 3. I do not think we should merge the formatting change at this time, and instead wait until everything required for v0.4 is completed, and the websites are redesigned.

I will return to this PR and explain the difference at a later date.

@asoplata
Copy link
Collaborator Author

I think we should use this PR as the main place to discuss agreeing upon a code Formatting-style . I will split changing our linter from flake8 to ruff check using a new PR that I haven't created yet, and remove its commits from this PR.

To actually define what I mean by a code "Formatting-style", and to avoid the inevitable word count limitations, I've written a Gist instead explaining how we could consider defining and communicating about Linting vs Formatting vs "Programming Style Guides", including with examples: https://gist.github.com/asoplata/8bb94707810218df95dacaaaaf34bccb .

@asoplata asoplata changed the title [WIP] Introduce ruff formatting, and replace flake8 linting with ruff linting [WIP] Introduce ruff code-format style Dec 13, 2024
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.

3 participants