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

Change imports to a one-per-line style #6755

Closed
xavfernandez opened this issue Jul 20, 2019 · 10 comments · Fixed by #6764
Closed

Change imports to a one-per-line style #6755

xavfernandez opened this issue Jul 20, 2019 · 10 comments · Fixed by #6764
Labels
auto-locked Outdated issues that have been locked by automation state: needs discussion This needs some more discussion type: maintenance Related to Development and Maintenance Processes

Comments

@xavfernandez
Copy link
Member

xavfernandez commented Jul 20, 2019

This would prevent useless merge conflicts.

I think it would only need to add force_single_line = true to our isort configuration.

Originally posted by @pradyunsg in #6729 (comment)

And for the record, I'm in favor :)

@triage-new-issues triage-new-issues bot added the S: needs triage Issues/PRs that need to be triaged label Jul 20, 2019
@xavfernandez xavfernandez added state: needs discussion This needs some more discussion type: maintenance Related to Development and Maintenance Processes labels Jul 20, 2019
@triage-new-issues triage-new-issues bot removed the S: needs triage Issues/PRs that need to be triaged label Jul 20, 2019
@xavfernandez
Copy link
Member Author

xavfernandez commented Jul 21, 2019

cc @chrahunt @cjerdonek @dstufft @pfmoore for your opinion :)

@pradyunsg
Copy link
Member

pradyunsg commented Jul 22, 2019

OT: that list is almost alphabetical and I don't enjoy that. 🙂

edit: I actually edited the list in the comment above to be alphabetical. Judge me internet. :P

@pradyunsg
Copy link
Member

FWIW, we'd probably use a style like:

from pip._internal.utils.misc import (
    _make_build_dir,
    ask_path_exists,
    backup_dir,
    call_subprocess,
    display_path,
    dist_in_site_packages,
    dist_in_usersite,
    ensure_dir,
    get_installed_version,
    redact_password_from_url,
    rmtree,
)

I'm almost certain that isort supports this as a mode in it -- 3 or 5.

(I typed this on Monday morning, on my mobile, while walking to class. I'm weird.)

I see there was some discussion at #6729 so I figured it'd help to clarify.

/cc @pfmoore

@pradyunsg
Copy link
Member

Filed #6764 for this.

@pfmoore
Copy link
Member

pfmoore commented Jul 22, 2019

I still don't like it, sorry.

@pradyunsg
Copy link
Member

pradyunsg commented Jul 22, 2019

Do you dislike it enough, to not be willing to work with it?

As such, this would prevent merge conflicts due on the import statements, which AFAICT is the most common kind of merge conflict we see in pip nowadays -- keyring PR had conflicts a lot of times and it was almost always only the imports.

@pfmoore
Copy link
Member

pfmoore commented Jul 22, 2019

I'm not looking to wield a one-man veto, no.

But my personal workflow means that I can't routinely run a tool like isort on changes I make, so something like this just adds an extra "manual edit, submit PR, get lint failure, manually fix it, rinse and repeat" cycle to my coding. So I want to register my preference, that's all.

@cjerdonek
Copy link
Member

I've never noticed / given any thought to the merge conflicts this is meant to address, so I don't personally see a need. But I can adapt to work with anything.

@pradyunsg
Copy link
Member

Thanks for clarifying @pfmoore! I wasn't intending to imply that, but then again, an explicit confirmation isn't a bad thing. :)

@cjerdonek, good to know! :)


I'm inclined to say let's do this. It's a less painful for external contributors, since their PRs are more likely to stay open for longer, which increases the likelihood of a conflict like this (eg. see the keyring PR or the 3.10 PR, which are really the main motivators for me supporting this change).

To that end, to keep us from stalling here, I'll wait for another couple of days and unless someone raises a "don't do this; it's bad" concern, I'll merge #6764.

@pfmoore
Copy link
Member

pfmoore commented Jul 24, 2019

Good point re reducing the pain for external contributors. I'll learn to live with the change, might even persuade me to improve my workflow, who knows? :-)

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Aug 25, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation state: needs discussion This needs some more discussion type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants