Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linter updates #8902

Merged
merged 3 commits into from
Sep 23, 2020
Merged

Linter updates #8902

merged 3 commits into from
Sep 23, 2020

Conversation

pradyunsg
Copy link
Member

Carries forward #8803

Toward #8543

@pradyunsg pradyunsg added skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes labels Sep 23, 2020
@pfmoore
Copy link
Member

pfmoore commented Sep 23, 2020

Drive-by comment - I really dislike the multiple changes to produce

from typing import (
    # ... many lines of names, one per line
)

rather than condensing them onto a single line. If there's any way to avoid that, I'd be really happy.

Hmm, maybe we add a file pip._internal.common_types and do from pip._internal.common_types import *. I bet black hates import * as well 🙁

@pradyunsg
Copy link
Member Author

pradyunsg commented Sep 23, 2020

rather than condensing them onto a single line. If there's any way to avoid that, I'd be really happy.

Upcoming changes will do that (isort's black configuration does what you want here).

@pradyunsg
Copy link
Member Author

Re-reading your comment, I'm actually a bit confused -- could you point to a specific line?

@pradyunsg pradyunsg mentioned this pull request Sep 23, 2020
@pfmoore
Copy link
Member

pfmoore commented Sep 23, 2020

Sorry, it was very drive-by. Here's an example: https://github.com/pypa/pip/pull/8902/files#diff-4d153bf2a557be9dae85e703bace82d8L33

I don't care a lot in practice (and I was pleasantly surprised that this was the only style choice I didn't like). I was mostly just flagging it on the basis that it might be a config that can be changed.

Looking at https://black.readthedocs.io/en/stable/compatible_configs.html#why-those-options-above it's discussing

from third_party import (lib1, lib2, lib3,
                         lib4, lib5, ...)

being invalid, and the recommended alternative is

from third_party import (
    lib1,
    lib2,
    lib3,
    lib4,
)

My (strongly) preferred alternative is

from third_party import (
    lib1, lib2, lib3,
    lib4,
)

(with the libX lines being wrapped when they exceed the normal line length limit, not one-per-line). That's pip's current style, and I prefer it over either of the two versions in the black documentation.

Looking at https://github.com/PyCQA/isort#multi-line-output-modes, I'm referring to isort mode 5 ("Hanging Grid Grouped").

@pradyunsg
Copy link
Member Author

pradyunsg commented Sep 23, 2020

That's a nightmare for merge conflicts though. :(

That formatting would allow for 2 PRs like:

from third_party import (
    awesome, lib1, lib2,
    lib3, lib4,
)
from third_party import (
    lib1, lib2, lib3,
    lib4, zebras
)

And I'm sure you can see why git won't like that. I think I'd initially added isort with mode 5, and we switched to mode 3 because of this issue.

@pradyunsg
Copy link
Member Author

FWIW, this PR isn't adding any new style choices -- the update to isort 5 means that we're now sorting all groups of imports, including those inside the if MYPY_CHECKING and what not, which was not doing in isort 4.

All the black-related style and mode changes can be found in #8903.

@pradyunsg
Copy link
Member Author

I think I'd initially added isort with mode 5, and we switched to mode 3 because of this issue.

#6755 -- yup. :)

@pfmoore
Copy link
Member

pfmoore commented Sep 23, 2020

That explains why it's a relatively small change. I'm not convinced by the git argument, it's more important to me that source code should be readable by humans than that computers have an easy time. And paging past a huge block of typing declarations at the top of the file damages readability, IMO.

But as I said on #8903, I don't have the energy to be a lone voice against the rising tide of auto-formatting enthusiasm. So 🤷 - I'll express my opinions in the future by swearing at black in commit messages 🙂

@pfmoore
Copy link
Member

pfmoore commented Sep 23, 2020

#6755 -- yup. :)

... and #6755 (comment) 🙂

@pradyunsg
Copy link
Member Author

It's almost as if our opinions were the same the last time we discussed this. ;)


from pip._internal.utils.temp_dir import (
from pip._internal.utils.temp_dir import \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch... shouldn't EOL escaping be banned?

@pradyunsg
Copy link
Member Author

It should, but that's isort doing this. :/

The black profile fixes this. 🤷🏻‍♂️

@pradyunsg
Copy link
Member Author

I'mma merge this, coz... uhm... it's "only" version updates to our existing linters. We can discuss changing configuration separately, since that's unrelated to actually updating them (mostly!).

@pradyunsg pradyunsg merged commit faee60b into pypa:master Sep 23, 2020
@pradyunsg pradyunsg deleted the linter-updates branch September 23, 2020 16:15
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants