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

Reformat the codebase and enable black check in CI #2750

Merged
merged 15 commits into from
Jun 14, 2023
Merged

Conversation

Helveg
Copy link
Contributor

@Helveg Helveg commented Apr 28, 2023

No description provided.

@Helveg
Copy link
Contributor Author

Helveg commented May 4, 2023

@heplesser @jougs If you could review the none-cake containing commits and merge this before merging any other Python-containing PRs then we have black in.

Prominent changes:

  • Added black pre commit hook and enabled CI check; dev docs on the topic was already included in the previous PR Separated checks #2590
  • Reformatted the Python codebase and added the commit sha to the ignore revs file so that blame isn't cluttered
  • Added E203 to the flake8 ignore list because it is not PEP8 compliant and Black and flake do not agree about it. With black in place, the relevance of a pep8 checker is dubious, because black conforms to PEP8 and leads to a single formatted outcome.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@Helveg Thank you for this PR. My main concern is that it reduces the line length to 80. While this is in line with PEP8, we have concluded that for C++ code we allow 120, because it makes code more readable. In the diff I noticed numerous lines that are now broken although just slightly over 80, not improving legibility. So I would strongly argue for using 120 also for Python code.

And could you elaborate on the SHAs? I don't quite understand what that is about.

@Helveg
Copy link
Contributor Author

Helveg commented May 8, 2023

Oh, I thought I had set the line length config to 120, I will double check! The SHA removes the commit from any git blame checking, otherwise my reformatting commit would show up everywhere as the person last changing that code, and more meaningful history and blame is occluded

@Helveg Helveg requested a review from heplesser May 9, 2023 09:55
@Helveg
Copy link
Contributor Author

Helveg commented May 9, 2023

@heplesser I force pushed a new attempt; the line length is now set in pyproject.toml.

Small question: why are so many tool settings set in tox.ini and not in pyproject.toml? Maybe this predates the pyproject.toml support?

@heplesser
Copy link
Contributor

@heplesser I force pushed a new attempt; the line length is now set in pyproject.toml.

Small question: why are so many tool settings set in tox.ini and not in pyproject.toml? Maybe this predates the pyproject.toml support?

@Helveg History is probably the most plausible explanation. I don't think it was an explicit choice of tox.ini in favour of pyproject.toml. I think when I first started with tox (in another context), I came across instructions that gave a minimal "do not change" pyproject file and lots of details of what to put into tox.ini. Feel free to create a new PR with a better structure (and ideally links to documentation of the file formats/purposes).

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

I have a few questions concerning places where code had been hand-formatted for readability. I have not systematically searched for all such cases yet, would like your feedback first.

pynest/examples/balancedneuron.py Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Copy link
Member

@nicolossus nicolossus left a comment

Choose a reason for hiding this comment

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

LGTM!

@Helveg
Copy link
Contributor Author

Helveg commented Jun 3, 2023

@heplesser I am surprised? So we are fine with disrupting all the currently aligned comments in the codebase?

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

I am fine with this change, but did obviously not look at all the changes

@jougs jougs merged commit 8f20b19 into nest:master Jun 14, 2023
@Helveg Helveg deleted the black branch June 14, 2023 13:37
@terhorstd terhorstd added T: Enhancement New functionality, model or documentation S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Enhancement New functionality, model or documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants