Code formatting - Black?

[sebhahn]

I saw that Black has been used lately for code formatting in this package. I'm not sure if this has been discussed, but personally, I'm not entirely happy with the approach of Black formatting. IMHO it is too strict and readability is suffering e.g. this one-liner ends up to be 9 lines. I understand that Black wants to make diffs as easy as possible, but maybe we can select a config that is less restrictive or use another code formatter. What do you think?

[wpreimes]

We have pre-commit hooks in place to check that the code is PEP8 conform. We recently used black (with line legth 79) for most files (but not all yet I think), but formatting with e.g. autopep would also work to make the precommit checks pass.
I will add a line to the developers guide on this, probably easiest if everyone simply uses the same command (i.e. black -l 79 file.py. We could think about checking the formatting when a PR is opened automatically, and then only accept PRs where the precommit hooks pass.

[s-scherrer]

I actually don't see a problem in the linked "one-liner". Maybe I'm already used to this, but on my notebook and using two-column mode in my editor, the alternative is a wrapping line, which reads much worse.

I'm fully open to using another autoformatter though.

[sebhahn]

IMHO the problem is that you can only see x lines at a time on your screen and if every statement is split for each single word the code overview suffers from it.

data = [
                resample_mean(
                    other_times,
                    other[col].values[mask],
                    ref_dr.to_julian_date().values,
                    window_days
                )
                for col in other
            ]

is inferior compared to

data = []
for col in other:
    data.append(resample_mean(other_times, other[col].values[mask], 
                              ref_dr.to_julian_date().values, window_days))

saving 50% of lines

[s-scherrer]

Fair point. In this example, I think one of the main issues is the code repetition in the block below (I wrote that code, so that's on me), but in general with functions taking a lot of arguments that can be an issue. Do you know an autoformatter which handles this better than black?

[sebhahn]

I'm blaming black for this not you 😄

W.r.t. function calls, the length of variable names are of importance, as well as whether it makes sense to use a dictionary as a collection of arguments. Latter should actually help to group a few arguments into a single variable.

I'm using autopep8, where it is possible to configure its behavior and e.g. exclude certain pep8 violations/warnings. Maybe there is already a new python package with better/more features (except for black)

[s-scherrer]

I'm blaming black for this not you

Haha, no worries. I was referring to the overall quality of this code, not the issue with the line breaks in the function call. I thought "no wonder this doesn't fit well on a screen if the idiot who wrote that code did the same long function call twice", but then git blame turned out to be very useful 😅

Do you mind sharing your configuration, or at least the relevant sections?

[sebhahn]

so far I only excluded E402 - Fix module level import not at top of file at some point, because there might be a use case where code should come before an import statement

[pycodestyle]
ignore = E402

[s-scherrer]

I just compared autopep8 and black on this line, starting from the one-liner version:

            data = [resample_mean(other_times, other[col].values[mask], ref_dr.to_julian_date().values, window_days) for col in other]

black produces 9 lines of code:

            data = [
                resample_mean(
                    other_times,
                    other[col].values[mask],
                    ref_dr.to_julian_date().values,
                    window_days,
                )
                for col in other
            ]

autopep8 produces 6 lines of code.

            data = [
                resample_mean(
                    other_times,
                    other[col].values[mask],
                    ref_dr.to_julian_date().values,
                    window_days) for col in other]

yapf would be an other option, and produces only 5 lines of code for this example:

            data = [
                resample_mean(other_times, other[col].values[mask],
                              ref_dr.to_julian_date().values, window_days)
                for col in other
            ]

Personally I find the autopep8 version the least readable here. Yapf performs quite well and has a few configuration options. I used --style='{based_on_style: yapf, indent_width: 4}' here.

Should we change to yapf then? @wpreimes

[wpreimes]

To me yapf and black look both good. If it's just for a few exceptional cases where we are unhappy with black, we could also exclude those lines from being auto-formatted / checked? But if @sebhahn is unhappy with black in general, we can use yapf here (in any case we should make sure everyone does it the same way -> developers guide).

[sebhahn]

@s-scherrer the fundamental problem of the example is that it is based on a list comprehension, the result would be different if it changed to a classical loop + list/append; yapf only produces 5 lines because the width of 80 characters is not forced

Based on my experience with black and yapf, I would favor yapf given that more configuration is possible.

@wpreimes, a demo yapf config file in (each?) the repo would be helpful

[s-scherrer]

As far as I understand it yapf also enforces the 80 width limit, but it's also possible to change this limit.

I added the following config to setup.cfg:

[yapf]
based_on_style = yapf
indent_width = 4
column_limit = 80

[sebhahn]

true, it is below 80 character 😯