Add initial black formatting

[a-feld]

Preface

This PR is not intended to be a discussion about style and this PR does not represent my personal opinions about black or formatting. It is simply meant as the initial entry into using an autoformatter which may be configured later as desired (as discussed in #88). As such, the default options for black the only setting for black is using line length of 79 in this PR.

Overview of Changes

Black has been configured in accordance with the current readme on the black github page. This includes:

• Setting the max line length to 88 where appropriate
• The recommended isort configuration is used for compatibility between black and isort

pylint and black seems to disagree on line continuations so the pylint bad-continuation rule has been disabled.

pylint and isort can disagree on import order so the pylint wrong-import-order rule has been disabled.

Line length lint checks have been disabled since black may not respect line limit. From the black readme:

Black will try to respect [line length]. However, sometimes it won't be able to without breaking other rules. In those rare cases, auto-formatted code will exceed your allotted limit.

[Jamim]

Hello @a-feld,
I believe that we must configure Black to use 79 as the line length number instead of changing max-line-length settings for flake8, pylint, and isort.

[a-feld]

Hi @Jamim, thanks for the comment!

I worry that changing the default line length may cause adverse interaction between black and isort; however, I'm not certain. That being said, the purpose of the initial PR is to just get black configured with known good default settings and then proceed from there.

I'm not making a judgement of what the line length should be, I'm just trying to get black formatting working reliably without introducing a burden on the maintainers.

[Jamim]

I've created the a-feld#1 PR that adds initial settings for Black.

[a-feld]

Thanks @Jamim , can we make that a follow on PR to this? Your input is valuable and I believe there will be discussion. I'm trying to keep the scope of this PR minimal in order to avoid discussion around line length, string normalization, etc.

[Jamim]

Thanks @Jamim , can we make that a follow on PR to this?

Unfortunately, no, we can't.
I believe that we have to minimize impact on the history just like described in this comment.

[toumorokoshi]

will this also impact the rules around only importing one object per line? Although I'm not particularly fond of the existing configuration, I think that choice should be discussed in a separate PR.

[c24t]

I don't see a way to get black and isort to work together with force-single-line.

Starting from:

from an.absurdly.long.path.that.forces.this.line.over.the.textwidth import module_a
from an.absurdly.long.path.that.forces.this.line.over.the.textwidth import module_b
from an.absurdly.long.path.that.forces.this.line.over.the.textwidth import module_c

isort will break lines with backslashes using the old config:

from an.absurdly.long.path.that.forces.this.line.over.the.textwidth import \
    module_a
from an.absurdly.long.path.that.forces.this.line.over.the.textwidth import \
    module_b
from an.absurdly.long.path.that.forces.this.line.over.the.textwidth import \
    module_c

but then black will replace those with parens:

from an.absurdly.long.path.that.forces.this.line.over.the.textwidth import (
    module_a,
)
from an.absurdly.long.path.that.forces.this.line.over.the.textwidth import (
    module_b,
)
from an.absurdly.long.path.that.forces.this.line.over.the.textwidth import (
    module_c,
)

...which isort will turn back into slashes, and etc.

I don't see an isort option that uses parens but doesn't group imports, so if we want to use black this may be the best we can do:

from an.absurdly.long.path.that.forces.this.line.over.the.textwidth import (
    module_a,
    module_b,
    module_c,
)

[toumorokoshi]

Overall the changes look good to me!

One thought: I'd switch the committer back to you for the commit, as the CLA checker cannot understand the committer. Despite it being a styling change, there is still some value in being able to point that the change was enacted by an individual.

[toumorokoshi]

Regarding the line length: I'm not clear on why using the default black line length is a bad thing. The more custom configuration we add, the more people have to adjust their editors. Although there is a pyproject.toml so maybe that's not so much an issue.

Separate from that, I don't see a strong justification for any line length besides preference as IDEs tend to vary wildly on the possible widths. Throwing in a vote to leave line length as the default set by black, I suppose.

[reyang]

Hey! I suggest that we stick with 79 since that's the current rule and so far we've been very consistent https://stackoverflow.com/questions/88942/why-does-pep-8-specify-a-maximum-line-length-of-79-characters.
The moment we start to talk about 88, other preferences would come up (for example, personally I prefer 120 for no obvious reason), and we end up bikeshedding.

[reyang]

Please stick with 79 instead of 88.

[carlosalberto]

The moment we start to talk about 88, other preferences would come up (for example, personally I prefer 120 for no obvious reason), and we end up bikeshedding.

Agreed, lets stay closer to the conventions as much as possible to stay out of style discussions ;)

[a-feld]

@carlosalberto @reyang Happy to change it, just wanted to point out that with the non-default line length many editors which use black will require configuration since the pyproject.toml is not usually respected (see the official vim plugin, for example).

To @toumorokoshi 's point,

The more custom configuration we add, the more people have to adjust their editors.

Does this change your position for initial configuration of black?

[reyang]

Does this change your position for initial configuration of black?

No it doesn't change my position. I don't have strong personal preference/opinion over styling as long as we're consistent :) My opinion is that given we've already decided to stick with 79, editor convenience wouldn't add too much weight to alter the original decision.

[a-feld]

@reyang @carlosalberto I just tried lowering the limit to 79. I'll push up those edits, but there's 1 example where line length is 80 and I'm not sure there's anything I can do about it.

From the black readme:

If you're paid by the line of code you write, you can pass --line-length with a lower number. Black will try to respect that. However, sometimes it won't be able to without breaking other rules. In those rare cases, auto-formatted code will exceed your allotted limit.

[a-feld]

I've disabled line-length lint checks in order to leave this up to black.

[Jamim]

Hi @a-feld,
Please try a-feld#2.

[a-feld]

Thanks @Jamim , I think the tests are passing now, so we should be good to go! 😃

[Jamim]

I think the tests are passing now

But they are passing due to line length check is disabled 😕

we should be good to go!

Personally I'm still quite unhappy about double quotes 😞

[Jamim]

I've disabled line-length lint checks in order to leave this up to black.

a-feld#2 fixes that line and re-enables lint checks.

[carlosalberto]

Btw, I still would like to get the opinion of @c24t (as he's been away for the last week)

[reyang]

LGTM.

[reyang]

Discussed in the community meeting, we'll merge this first.
Whether single quote or double quote is preferred can be a separate discussion.
At this stage, we don't worry too much about history - because we don't have a lot history.

[c24t]

I still would like to get the opinion of @c24t (as he's been away for the last week)

I think there's something to infuriate everyone in black. I personally like using double quotes for text-like strings and single quotes for symbol-like strings, but don't generally comment on others' use of quotes. As @toumorokoshi says in #88, that's a class of discussion that doesn't provide material value to PRs. I think it's worth turning this decision over to the formatter, even if it makes for uglier code.

I wouldn't worry about preserving history too much at this point. We're still early in the project, and likely to have more cross-cutting changes like this soon. Better to make this change sooner rather than later.

Setting the line length to 79 seems like the right choice to me for the reasons that @reyang notes above. I understand black is supposed to be uncompromising, but 88 is a psychotic default.