Reformat code with black and ruff #201
Conversation
| @@ -311,15 +245,13 @@ def crop( | |||
| keep_non_selected: bool, | |||
| comment, | |||
| buffer, | |||
| debug | |||
| debug, | |||
There was a problem hiding this comment.
should that comma be there?
There was a problem hiding this comment.
Trailing commas are a Black thing which allows a user to add a comma at the end of a multiline list to tell Black not to format it back to a single line, if it could fit within the line length. However, it comes with issues and I've now updated the config to skip this "magic" functionality.
| def __init__( | ||
| self, | ||
| stops, | ||
| hour, | ||
| minute, | ||
| o_zone, | ||
| d_dist, | ||
| d_freq, | ||
| facility_sampler, | ||
| activity_params, | ||
| threshold_matrix=None, | ||
| threshold_value=None, | ||
| ): |
There was a problem hiding this comment.
everywhere else the signatures are collapsed to be inline but it's the opposite here - a bit weird
There was a problem hiding this comment.
if > 120 lines, it has to break the sigs. No other function in the file has enough sigs to go over 120 lines.
tests/test_19_combining.py
Outdated
| # assert os.path.exists(os.path.join(path_output_dir, 'plans.xml')) |
There was a problem hiding this comment.
surprised this commented out test wasn't deleted
There was a problem hiding this comment.
This would be based on running ruff with rule ERA, but it isn't very clever and doesn't properly eradicate the whole commented-out code block. I'll delete it manually.
So I chose 120 characters based on:
My personal preference is based on a line width that uses available screen real-estate, but without having to horizontally scroll, even with two windows of code side-by-side on the screen. I spent years working on projects that used 80 characters, and over time found that:
Pep8 does suggest 79 characters, but it also says:
I think line length is somewhat arbitrary, as the various differing defaults demonstrate. The 80-character limit probably dates back to the optimal width of punchcards, later carried over to terminals. Even I wasn't around for that era... I recognise that it's sometimes contentious, and personal preferences play a large part. If there is a strong universal desire to move to 80/other characters, I wouldn't object, but if not, I'd rather keep to 120. |
|
Agreed, mostly personal preference. Avoiding long names and weird line breaking should be solved by just letting automatic formatters deal with all that automatically on commit/save. This is why I like 88 characters (the Black default - rulers for 80 and 88 characters shown): 
On my 14 inch device I can have two files side by side (e.g., source + tests) on one screen. Even without the directory tree on the left, 120 character lines would be wrapped. Admittedly, on a 24-inch screen, the 120 character width is fine with two files side-by-side:
|
|
Here is the argument for 88 characters: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length |
|
With my side-by-side on a 14-inch screen argument, I could go up to 100 characters... Maybe a compromise...? |
I'm happy with that if nobody else objects 👍 |
Fixes #144.
I have reformatted the code using black and ruff and extended the linting rules which will apply in future QA. This brings the code more in line with the pep8 python code style guidelines.
There are many more linting/code style rules that could be applied, beyond the base ones I've applied. However, the more that are added, the more that the current code fails to adhere to, requiring manual code edits to fix (rather than what have mostly been automatic fixes on my application of
ruffhere).I would say we can extend the applied rules as and when we find time to update the code to adhere to those rules. One obvious one is adding docstrings for all public functions/modules, which we currently don't enforce.
I would also prefer to bring the line length down from 120 to ~90 lines (the pep8 standard is 79, black defaults to 88). Is there a reason for the 120 lines?