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

Link isort profile to Black code style mention #2246

Merged
merged 1 commit into from May 19, 2021

Conversation

felix-hilden
Copy link
Collaborator

@felix-hilden felix-hilden commented May 18, 2021

Hi, the isort configuration currently in the Black code style document is duplicated in Using Black with other tools. I think it would be better to consolidate information and simply link to the tool guide, mentioning the easy profile in the original document.

I think skip news, what do you think about the change?


I changed the link to isort PyPI page because for users it could be better to see the Black docs on why that configuration is necessary and what isort is from Black's perspective.

@JelleZijlstra JelleZijlstra added the skip news Pull requests that don't need a changelog entry. label May 18, 2021
@ichard26
Copy link
Collaborator

Thanks for the heads up! I'll review this soon, just a little busy today. Although, what is CI doing!? Why is everything cancelled? I guess it's because they're all very stuck.

Nevermind... there's an active incident with GitHub Actions. Time to wait then huh.

@felix-hilden
Copy link
Collaborator Author

It seems that the issue is resolved and Actions is recovering!

@ichard26
Copy link
Collaborator

I reran the jobs and they still failed. I don't think GHA has fully recovered yet :(

Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! LGTM, I like reducing duplication in the docs :)

@ichard26 ichard26 added C: cleanup Refactoring and removing dust :) T: documentation Improvements to the docs (e.g. new topic, correction, etc) labels May 18, 2021
@ichard26 ichard26 merged commit 3bba808 into psf:main May 19, 2021
@felix-hilden felix-hilden deleted the isort-config-link branch May 26, 2021 08:17
@WhyNotHugo
Copy link
Contributor

Is that profile considered part of black's style or isort?

I'd like to bring up for discussion the idea of changing that profile to use force_single_line=true, which reduces merge conflicts by a lot (I feel this aligns with black's approach too).

However, I'm not sure if I should bring it up here, or it isort's tracker.

@ichard26
Copy link
Collaborator

ichard26 commented Jul 25, 2021

It's probably better if you bring it up on the isort issue tracker since we don't know isort well. I had to search up with force_single_line is. FWIW, it doesn't seem like something Black would learn to enforce.

I wouldn't mind being CCed in though.

@MarcoGorelli
Copy link
Contributor

which reduces merge conflicts by a lot (I feel this aligns with black's approach too)

yeah, imports were a major source of merge conflicts in pandas, setting force_grid_wrap helped resolve this. It works fine with profile=black

https://github.com/pandas-dev/pandas/blob/daec2e73b48fe34086dcdeeb0858d29536b6ca6a/pyproject.toml#L115

However, I'm not sure if I should bring it up here, or it isort's tracker.

pretty sure this would be isort, as that's where the profile's defined

@WhyNotHugo
Copy link
Contributor

force_grid_wrap can have weird merge conflicts though, whereas force_single_line is a bit more dead simple to deal with.

I've opened an issue to discuss the topic on the isort side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: cleanup Refactoring and removing dust :) skip news Pull requests that don't need a changelog entry. T: documentation Improvements to the docs (e.g. new topic, correction, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants