Reformat the code with black

[dstufft]

No description provided.

[dstufft]

This has two commits, the first one is the actual logical changes to make black part of our build chain. It adds black to make lint, and adds a new command, make reformat. The second commit is just making the rest of the code valid so that this PR passes.

[di]

I like it. I'm already using black locally so adding it here makes things easy for me. Guess I'll be making the switch to 88 character line widths...

[dstufft]

This is probably blocked on psf/black#64 and psf/black#22 to be resolved before we can use this.

[dstufft]

This now has some manual reformatting (prior to the make reformat) that works around psf/black#64. The remaining linting errors all come from psf/black#22. We could use some comment blocks to prevent black from reformatting those if we wanted though.

[brainwane]

Comment from @sigmavirus24 on the list:

If you look at the issues I'm not sure it's presently stable enough for Warehouse. There are quite a few bugs which is normal for such a new project

[ambv]

I'm very happy you're testing Black. I would wait a while before jumping in, at least until beta 1. I will let you know as soon as the current blockers are resolved.

[brainwane]

Thanks, @ambv.

@njsmith on the mailing list recommends we consider yapf.

My suggestion: @dstufft could redo this PR so we retain the code reformatting but take out the dependence on black, merge it, and then keep an eye on black and yapf and similar stuff, and submit a new PR to add one of them.

[dstufft]

I'd rather not merge a bunch of formatting changes without a mechanism to ensure things stay consistent going forward. Merging this will conflict with open PRs, that's OK if the pay off is "then you never have to think about formatting again", but less so if the pay off is just "and now things are formatted differently".

[ambv]

I agree with Donald. Wait a few more days, the main two missing pieces in Black are:

• consistent treatment of comments within bracketed expressions; and
• automatic placement and removal of organizational parentheses (mostly for use in imports, if statements, for and while loops, assignments, and assert statements).

I want to get to the first within the next few days and tackle the other over the weekend. Hopefully the reformatting of Warehouse then will prove good enough to adopt the tool.

Afterwards, barring small bugs, we will only be left with one planned feature that's currently missing: standardizing all strings to double quotes (unless those introduce escapes). This is needed to minimize backslash escaping and to be able to join "implicitly" "concatenated" "strings" "on 1 line".

[brainwane]

I skimmed the black commit log and think the three items @ambv mentioned on March 28th are now done:

psf/black@cb5aada
psf/black@80bd2b3
psf/black@c55d08d

[dstufft]

Ah, there's another blocker to this too that I forgot to mention: psf/black#67. The outcome of which will likely decide whether I'm willing to actually use black on this code base or not. I've detailed some more specific thoughts at psf/black#67 (comment).

I mentioned it in the comment thread on black, but I want to re-iterate since this issue is linked from black's repository now and folks have sometimes tried to use my statements as cudgels against other projects. I think it is perfectly acceptable and fine if black decides to go in a way that makes me unwillingly to use it for this specific repository. Opinionated projects will always sometimes not be the right fit, or produce worse results, for some circumstances and if the opinions of black end up making that true for Warehouse, that is perfectly OK. Of course I do hope we can use it here, but the decision of what opinions black has is up to the black project, not me.

[dstufft]

I've updated the pinned version of black in this PR, removed the manual formatting tweaks I had to do to satisfy the other bersion, and reformatted the project using black 18.4a2 to give folks a chance to poke around the way black would currently format the project and see if they see any major issues with readability or what have you. This version still does not pass our normal linter (when relaxed to 88 characters) for two lines of code:

$ make lint
/Users/dstufft/projects/warehouse/.state/env/bin/flake8 .
./tests/unit/i18n/test_init.py:47:89: E501 line too long (90 > 88 characters)
./warehouse/cache/http.py:61:89: E501 line too long (90 > 88 characters)
make: *** [lint] Error 1

The first of these errors is a block of code that looks like:

def test_includeme():
    ...
    assert (
        config_settings
        == {
            "jinja2.filters": {
                "format_date": "warehouse.i18n.filters:format_date",
                "format_datetime": "warehouse.i18n.filters:format_datetime",
                "format_rfc822_datetime": "warehouse.i18n.filters:format_rfc822_datetime",
                "format_number": "warehouse.i18n.filters:format_number",
            }
        }
    )

The line that fails is the longest line in the dictionary, the one for the key format_rfc822_datetime". I'm not entirely sure a good, non ugly way to resolve this ourselves. Maybe @ambv could chime in to say if that behavior is expected, or if black should have split the value apart to get it under 88 characters.

Playing around with it a little bit, it appears that you can get it to give you better behavior by wrapping with parentheses:

def test_includeme():
    ...
    assert (
        config_settings
        == {
            "jinja2.filters": {
                "format_date": "warehouse.i18n.filters:format_date",
                "format_datetime": "warehouse.i18n.filters:format_datetime",
                "format_rfc822_datetime": (
                    "warehouse.i18n.filters:format_rfc822_datetime"
                ),
                "format_number": "warehouse.i18n.filters:format_number",
            }
        }
    )

That feels like a bug with black though, since AIUI it's not supposed to care about the already existing formatting at all, and it should have already injected those parens. I wonder if @ambv would be willing to chime in about if that is a bug or not?

The other bit is code that looks like this:

def cache_control(
    seconds, *, public=True, stale_while_revalidate=None, stale_if_error=None
):

    def inner(view):

        @functools.wraps(view)
        def wrapped(context, request):
            response = view(context, request)

            if not request.registry.settings.get("pyramid.prevent_http_cache", False):
                if seconds:
                    if public:
                        response.cache_control.public = True
                    else:
                        response.cache_control.private = True

                    response.cache_control.stale_while_revalidate = stale_while_revalidate
                    response.cache_control.stale_if_error = stale_if_error
                    response.cache_control.max_age = seconds
                else:
                    response.cache_control.no_cache = True
                    response.cache_control.no_store = True
                    response.cache_control.must_revalidate = True

            return response

        return wrapped

    return inner

The line that fails is the stale_while_revalidate line. That is obviously stress testing the system since the variable names are super long here and there isn't a whole lot that black could reasonably do (especially in the generic case, where the left side of the expression could even be too long). For this case I think it's pretty easy to refactor the code slightly so that we don't have such a long line, something like:

def cache_control(
    seconds, *, public=True, stale_while_revalidate=None, stale_if_error=None
):

    def inner(view):

        @functools.wraps(view)
        def wrapped(context, request):
            response = view(context, request)

            if not request.registry.settings.get("pyramid.prevent_http_cache", False):
                cache_control = response.cache_control

                if seconds:
                    if public:
                        cache_control.public = True
                    else:
                        cache_control.private = True

                    cache_control.stale_while_revalidate = stale_while_revalidate
                    cache_control.stale_if_error = stale_if_error
                    cache_control.max_age = seconds
                else:
                    cache_control.no_cache = True
                    cache_control.no_store = True
                    cache_control.must_revalidate = True

            return response

        return wrapped

    return inner

Is a perfectly reasonable change to make to keep the lines shorter.

[dstufft]

Manual tweaks from above have been added in 3cfc497 to keep them separate.

[dstufft]

Poking around, I've found a few more potential issues from looking at the output produced by black: psf/black#153, psf/black#152.

Everything else though is look pretty fine to me. Some of it I would have done differently, but part of the point of using black is to stop having nit-picky code local opinions ;)

[ambv]

Thanks for testing it! I hope this is going to be mergeable soon!

Managing parentheses is a long subject. tl;dr - I can't always add or remove them automatically. Do it yourself and Black will keep it. It will become smarter about this in the future.

Longer explanation

Parentheses aren't entirely "existing formatting". Sometimes they are are needed just for human readability. Examples include:

• parentheses around logic expressions which aren't required by Python but make it clearer to readers what happens first;
• tuples in most places.

The formatter cannot decide whether something is easy or hard to read by humans so it doesn't remove those sorts of parentheses. In fact, for one-tuples, it's always adding them because they sometimes hid bugs.

Currently there's five kinds of expressions where Black will automatically manage parentheses:

• if (...):
• while (...):
• for (...) in (...):
• assert (...), (...)
• from X import (...)

That means it will add and remove parens based on whether they're needed or not in those cases. I could easily add support for this also around dictionary values and assignments. This would solve both of your issues reported above. However, I'm not entirely happy with what Black is doing with automatic parentheses. Since the algorithm is deliberately simple, it puts extra newlines in ways that human programmers wouldn't. Your assert example is in fact a nice illustration of what I mean.

So, before I add automatic management of parentheses in assignments and dictionary values, I need to improve how Black is formatting code in this case. (This will also improve formatting in cases like your assert above.)

In the mean time, as you observed, putting parentheses manually works around the problem.

[dstufft]

@ambv Ah okay, that makes sense! It also means that I'm probably going to have to format the files with black one at a time so I can actually review the diff of each file and make sure we're not introducing something crummy that can be solved by adding parentheses. That is more work, but not unreasonable (and it's really only meaningfully new work for me on this PR because we're reformatting such a large bunch of code, instead of just the smaller bit we've recently added).

Thanks a lot for all your help thus far!

[dstufft]

This should be ready now! I'd love to get @ewdurbin and @di to look at it to make sure they are happy with how things are formatting. There are a couple of known issues which are (reasonable) limitations in black combined with some structure of the code that already exists.

These will largely just require going through and doing some manual tweaking. The big one that I'm aware of is that black does not automatically combine string literals (because it would produce different AST). So in cases where we used to have something like:

foo = (
    "This is a "
    "string."
)

black then correctly combined that to a single line that looks like:

foo = "This is a " "string."

I've opt'd not to fix that in this PR, because this PR is hard to keep up to date as changes land and I have to rebase it. It's not bad if I can just blow away the reformat commit and reformat again, but if I have to re-apply manual tweaks each time that gets much more diifficult. Additional PRs can clean up the cases where we can get a better result out of black with some manual restructuring.

All in all, I am in favor of this PR and would like to get it landed.

[ambv]

This is exciting :-)

[dstufft]

I'm going to go ahead and merge this, Ernest is out and I believe he's already been +1 on this in general.