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

Use automatic code formatting (e.g. black?) #1295

Closed
mstimberg opened this issue Apr 30, 2021 · 3 comments
Closed

Use automatic code formatting (e.g. black?) #1295

mstimberg opened this issue Apr 30, 2021 · 3 comments

Comments

@mstimberg
Copy link
Member

As @denisalevi remarked in brian-team/brian2cuda#207, the code style in Brian's code base is far from consistent. One option would be to switch to automatic code formatting that would be applied for each commit (needs to be configured once at the user's machine) and gets checked in a GitHub Action. I'm still a bit torn about using black, because some of the decisions are "opinionated" (e.g. for lists or function parameters that span several lines, black will always add a trailing comma to make diffs for future additions smaller), and there are certainly cases where manual formatting can make the logic/structure more apparent. As others noted, black's "focus is on consistency, not readability", and it is not clear whether this is really a good thing... Also Guido says: "Black is overrated unless your team argues over style a lot" :-)

There'd be of course some intermediate solution, we could e.g. use tools like flake8 and isort which we could configure to our liking to be close to our current style, and which wouldn't necessarily reformat everything.

One definitive advantage of using some automatic tool: we wouldn't have to nitpick about code formatting during code review, and saying "please fix all the problems noted by the flake8 bot" would be certainly easier than saying "please read PEP8" or fixing things ourselves.

@denisalevi
Copy link
Member

denisalevi commented May 2, 2021

I have no strong opinion on the coding style itself. I would just really like some kind of consistency in brian2cuda, which I'm missing entirely at this point. And if the style is consistent with brian2, even better. I just don't want to spend much time on the details of it, hence my suggestion for black.

But brian2cuda also doesn't have a specific coding style really (which it probably different for brian2). Since it wasn't me who started writing the code, I never enforced a specific style but instead did something between "format to my liking whenever I do changes anyways" and "stick roughly to the style that the file has already". Throw in some "what I like changes over time" and you get the mess I have right now ...

So I really just want to run some formatter on the brian2cuda code with minimal manual intervention and be done with it (of course, there will have to be some manual intervention, especially for places where C++ code is written in Python strings).

That formatter doesn't have to be black, I can use autopep8 to automatically format code according a flake8 configuration file. And that same configuration could also be used as a GitHub action for PRs. The same is probably true for isort.

If you want to format brian2 code, I'd be happy to help configuring flake8 to your liking and use that for brian2cuda. If you would prefer to not mess too much with the brian2 style that you have right now, I would just run black or some default flake8 configuration on brian2cuda.

If you want to set up a style file, I could open a brian2 PR to see how different flake8 options affect brian2 code and to configure it to induce as little changes as possible. (Not intending to at a style change PR which would accredit me disproportionate code contribution, I mean just for seeing the effect of the configuration, I think you should commit any style change if that happens of course :P)

@oleksii-leonov
Copy link
Contributor

@mstimberg, I believe black is a good option and is the de-facto community standard for formatter. We are using black in multiple projects and more than happy with it.

For the cases when manual formatting makes sense (for example, tables), # fmt: off instructs black not to touch this part of the code.

Black reformats entire files in place. It doesn’t reformat lines that end with # fmt: skip or blocks that start with # fmt: off and end with # fmt: on. # fmt: on/off must be on the same level of indentation and in the same block, meaning no unindents beyond the initial indentation level between them.
https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html

My proposal for formatting enforcement with black#1435.

@mstimberg
Copy link
Member Author

Closed via #1435 (and later refinements)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants