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

Add automatic code formatting for Python code base and also generated standalone code #207

Open
denisalevi opened this issue Apr 29, 2021 · 2 comments

Comments

@denisalevi
Copy link
Member

The code style / state in brian2cuda is driving me nuts. I don't know how I survived looking at it for so long... ^^ And changing it as I go was also not the best idea... Now coding style is even inconsistent across the code base. I've always wanted to add some automatic code formatting such that the code just looks good and consistent.

And now that @SudeshnaBora is also working on this code, it becomes even more apparent that people use different coding styles and automatic formatting would just make me happier and everybody's life easier.

I have looked into formatters in the past and mostly gave up when being faced with format style decisions and time-requirements for setting things up. But recently, I came across black and started using it for formatting my own Python code. Simply because it does not require any decision on format, it simply chooses one. It is PEP 8 compatible. I don't have to give it a second thought. Code looks good. And I'm happy with it.

So, I'd like to format all Python code in brian2cuda with black and add it as a pre-commit hook, such that it is run automatically at every commit.

And while we are at it, I would also love to have an option to automatically format all generated standalone code. When working on a feature, I often first work on the generated code and my eyes are usually bleeding from it. And I never know if I should format the templates such that the templates look good or the generated code looks good (which is basically impossible). This would allow to do the former and still have formatted generated code.

@mstimberg Did you ever consider this? I think inconsistent coding style is also apparent in the brian2 repository. Even though everything should adhere to the basic PEP8 guidelines (that you also have in your developer guide), there are some aspects which are up to the user. E.g. the text width used varies across files and I remember being able to classify a file's author (meaning you or @thesamovar) based on the coding style :D

And for generated code, I'd have to look into it, but adding some brian2 build_option like format_code=True/False that can be set if desired could be a sweet addition. Might need an optional dependency for formatting, something like clang-format (but I haven't looked into that much yet). Generally, would you be interested in having such a thing in brian2?

@mstimberg
Copy link
Member

@mstimberg Did you ever consider this? I think inconsistent coding style is also apparent in the brian2 repository. Even though everything should adhere to the basic PEP8 guidelines (that you also have in your developer guide), there are some aspects which are up to the user. E.g. the text width used varies across files and I remember being able to classify a file's author (meaning you or @thesamovar) based on the coding style :D

We thought about it at some point I think. Back at the time I wasn't a huge fan of black and its "absolutist" approach, though. I still think there are several places where you can do nicer manual formatting that is PEP8-compatible but not "black-style", in particular for things like lists/arrays with parallel elements, or function parameters with some logical structure, etc. That having said, I agree that our general formatting is a bit of a mess and running black over it would almost certainly do more good than harm... I was always weary to do a big "all-at-once" code style change in the repo since it messes up git blame, but on the local machine there are ways around it, and it is not as if I'd use it very often in the first place. Food for thought, I'll open an issue for Brian to discuss it.

And for generated code, I'd have to look into it, but adding some brian2 build_option like format_code=True/False that can be set if desired could be a sweet addition. Might need an optional dependency for formatting, something like clang-format (but I haven't looked into that much yet). Generally, would you be interested in having such a thing in brian2?

I thought about this as well, and I agree that it could be nice in some situations. But it would certainly be a niche-feature that is only relevant to developers, so I wouldn't add a lot of (potentially hard-to-maintain) code for it – I certainly don't want to spend any development/testing time to make this work for different formatters, platforms, etc. 😄 A good middle ground could be something like a build option or preference that simply consists of a command line string that gets called for each file (e.g. something like "clang-format -i") – installing this, putting it into the path, etc. would be completely up to the user. And we could potentially only activate it if debug=True is used at the same time? Feel free to open an issue/PR about it!

@denisalevi
Copy link
Member Author

We thought about it at some point I think. Back at the time I wasn't a huge fan of black and its "absolutist" approach, though. I still think there are several places where you can do nicer manual formatting that is PEP8-compatible but not "black-style", in particular for things like lists/arrays with parallel elements, or function parameters with some logical structure, etc. That having said, I agree that our general formatting is a bit of a mess and running black over it would almost certainly do more good than harm... I was always weary to do a big "all-at-once" code style change in the repo since it messes up git blame, but on the local machine there are ways around it, and it is not as if I'd use it very often in the first place. Food for thought, I'll open an issue for Brian to discuss it.

The only reason I suggested black is that I don't want to spend time configuring a coding style myself. I don't care so much about the details of the style as long as it is consistent :) This is kind of the reason why I'm asking. I'd prefer to have a similar style in brian2cuda as in brian2. If you have style preferences, I'm happy to go with that. I just wouldn't know where to start. I'll comment on your brian2 issue.

And for the git blame issue, that is right. Unfortunately, the blame on GitHub would be a bit messed up by big style changing commits. Are you using the GitHub interface for blame though? I don't, I do that locally, and for that you can ignore specific commits as you said. I do that in .git-blame-ignore-revs. And there is also a GitHub support post / feature request to have it implemented on GitHub as well. And even if it isn't implemented, one can always click on "Parent commit" to find the right commit.

I thought about this as well, and I agree that it could be nice in some situations. But it would certainly be a niche-feature that is only relevant to developers, so I wouldn't add a lot of (potentially hard-to-maintain) code for it – I certainly don't want to spend any development/testing time to make this work for different formatters, platforms, etc. smile A good middle ground could be something like a build option or preference that simply consists of a command line string that gets called for each file (e.g. something like "clang-format -i") – installing this, putting it into the path, etc. would be completely up to the user. And we could potentially only activate it if debug=True is used at the same time? Feel free to open an issue/PR about it!

Yep, I agree with you that it shouldn't be creating additional work. It is certainly low priority for brian functionality, which is why none of us has implemented this so far I suppose :P. But it would just make me a bit happier. So I might put in an hour to get something working along the lines you suggested.

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

No branches or pull requests

2 participants