Initial python linting #282
Conversation
Split the "unit-tests" action into multiple actions, currently one for each package contained within the `flepiMoP` repo. Also updated checkout from v3 to v4 to address node16 deprecation warnings and swapped ubuntu 20.04 for ubuntu latest. Changed the gempyor ci to not print stdout and exit on first failure.
Typo in `setwd` call causes error about not being able to change to directory that doesn't exist.
Set the shell to bash so the `source` function is available.
Add the same path related limits from the on push to on pull_requests as well.
Removed the `breaking-improvements` branch from special consideration in GitHub actions.
Added black python formatter to check all python files in `flepiMoP` repo on push/PR to main/dev that edits a `.py` file. Left the action general enough to be updated for future linting additions.
Corrected formatting in all `.py` files using black formatter with command `black .` and now passes `black --check .`.
Added a few more examples of commands to help jump start users into using black when working on python code for `flepiMoP`. Also added a note leaving the door open for future additions.
TimothyWillard
added
documentation
Fix missing `with` in black formatter check step to contain `src` and `options`.
| # Reformat the python files automatically | ||
| black . | ||
| # Check if current work would be allowed to merged into flepiMoP | ||
| black --check . |
There was a problem hiding this comment.
I changed this to use the default line length (88 I think) and I assumed it wouldn't be a problem as I didn't think code formatting was actively being used, but I was wrong: #296 (comment). Do we have strong opinions about keeping this at 120? That's a ~33% increase over the default, which seems too wide to me. Definitely would make diffing two files painful or even viewing a file on the go a bit annoying. However, if there is strong consensus for the 120 I can change this back both here and in the CI action.
CC: @fang19911030, @jcblemai, @pearsonca
There was a problem hiding this comment.
classic line limits derive from punchcard languages, but modern screen dimensions / human reading comprehension tend to favor shorter line length limits. I recommend 72 or 80, which are the common cross-language defaults, inherited from fortran - not aware of python stretching those, but could be?
There was a problem hiding this comment.
I don't have a preference for these two numbers. I am happy with both of them.
There was a problem hiding this comment.
4 spaced tabs are used for indentation, so for a method inside a class with say a nested if (or for and then if) that's already 16 characters used up, I think something like 72-80 would leave too little space after that? But I'd be curious to hear other's thoughts. Pylint's default is 100, which is too long as well.
Looping in @emprzy, @shauntruelove, and @saraloo if y'all have any thoughts.
There was a problem hiding this comment.
I don't have any strong opinions. I agree that 72 is pretty short, but I don't really have too much of a dog in this fight either way.
There was a problem hiding this comment.
I really like longer lines, especially as we have a large space indentation. Otherwise, it makes code in things like step_rk4 very hard to read and diff. 120 has been chosen for this.
|
I think most of the code except the last 2 month of PR has been pretty consistently updated with black, so the still guide is not out of date, but yes, I wanted to ask you about enforcing it a pre-commit hook. I know there are more fashionable tools that black at the moment so feel free to change that. I feel quite strongly that the code is very hard to read with a line width of 80, see screenshoot (with black which default to 88), and uses a lot of vertical space per line. I think the default are generally too low especially since everyone has hidpi right now, which is also why the black dev decided to use 88 at their default (despite their complete respect for PEP8; that makes sense though as line lenght also the most controversial PEP8 choice -- and conflict with the give variable proper names thing), and why they also allow changing the line lenght as the only possible option at all despite their fordism :) what step_rk4 look like at 88: Maybe 120 it's too much, but I would go lower than black default of 88. I think with increasingly ~16:9 screen ratio, the vertical context lenght becomes very important. EDIT: fwiw with an normal windows on a MacBook Pro 14, with all the vscode side thing, I have 233 characters in font 12 printed, so it's true that for longer lines on 120, the diff will be affected. Another datapoint is that a modern language like julia uses 92 as the default. But they say it's arbitrary |
There are definitely tools that do more than On the line length issue, I think 120 is too wide. Could we come to some middle ground like that 92 number? I think the 120 is fine when editing a single file, but becomes painful when trying to view files side by side or do a review on GitHub. |
|
Yes 92 would be acceptable (even though I'd prefer 100). step_rk4 would need some welcome variable name change (index > idx) to make it readable at that lenght, but I've checked most of the code and it looks well on 92 with black. Perhaps my font are small but I frequently have screen split in 3, however I agree that github is a problem. |
|
Replaced by GH-358. |
Describe your changes.
This PR takes the first steps at:
Interestingly there was already a note in the GitBook documentation about code formatting, but it appears that it was out of date and never enforced. This PR edits a large number of files because of (2), but the two files to pay attention to in particular are:
.github/workflows/code-linting-ci.ymlanddocumentation/gitbook/development/python-guidelines-for-developers.md.I've also left the code linting action general enough to take on other steps as well in the future (stricter linting via
pylint, R code linting vialintr, etc.).What does your pull request address? Tag relevant issues.
This PR addresses GH-279 and is blocked by GH-280 (hence marking as draft for now).
Mentions of relevant team members.
@shauntruelove, @pearsonca, @jcblemai