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

More prominent black compatibility documentation #1543

Closed
timothycrosley opened this issue Oct 8, 2020 · 9 comments
Closed

More prominent black compatibility documentation #1543

timothycrosley opened this issue Oct 8, 2020 · 9 comments
Assignees
Labels
black Black compatibility issue documentation Improvements or additions to documentation enhancement New feature or request Hacktoberfest

Comments

@timothycrosley
Copy link
Member

timothycrosley commented Oct 8, 2020

There has been some confusion about how to make isort compatible with Black. As Black is quickly, and for good reason (IMHO) becoming the dominant source code formatter for Python, isort should prominently display (above the fold) how to make the two projects compatible. For most users this simply means isort --profile black. Details should include when used with pre-commit, set via a config, etc.

@timothycrosley timothycrosley added enhancement New feature or request black Black compatibility issue Hacktoberfest documentation Improvements or additions to documentation labels Oct 8, 2020
@Bhupesh-V
Copy link
Contributor

Bhupesh-V commented Oct 11, 2020

Up for grabs?
3 things to add:

  • Basic compatibility (black v/s isort)
  • Integration with pre-commit
  • Using a config file (.isort.cfg)

Anything else?

@timothycrosley
Copy link
Member Author

@Bhupesh-V yep! Feel free to work on this, and I think that sounds right to me.

Thanks!

@Bhupesh-V
Copy link
Contributor

@Bhupesh-V yep! Feel free to work on this, and I think that sounds right to me.

Thanks!

Ok thanks, I will come up with a draft PR soon

@ssbarnea
Copy link
Member

I am quite curious about mixing pre-commit, black and isort as I recently encountered some problems where isort seems to make changes but only on CI.

@timothycrosley
Copy link
Member Author

timothycrosley commented Oct 13, 2020

@ssbarnea are you able to provide additional information about this case? I'd encourage you to open a new issue for it providing:

  1. The output of isort . --show-settings
  2. A list of all the ways you run isort. How does it run with pre-commit? How does it run on CI/CD?
  3. The version of isort

Running isort, with the same file and settings, should be guaranteed to produce the same output. However, if you use pre-commit for local, and CD runs it directly, it's very easy for an incorrect configuration or simply different configuration to be passed to one vs the other.

#1549 is a good example of this. pre-commit doesn't offer a clean way to sort all file types that isort supports, so a lot of pre-commit setups will only sort .py and "succeed" locally, but then fail in CD when it sorts .pyi and .pyx files.

@AndreCimander
Copy link

@Bhupesh-V Many thanks for doing this 👍

Might I recommend adding a strong hint about using --filter-files in the pre-commit hook and skip_glob in the config file? One of my clients had a frustrating experience trying to exclude certain files using the pre-commit hook and I also had to read up on it.

@timothycrosley
Copy link
Member Author

@AndreCimander that's a good call out, and another area where calling via isort directly and calling via pre-commit are not identical. It may make sense for there to be a dedicated pre-commit guide as well (beyond just for black compatibility)

@timothycrosley
Copy link
Member Author

timothycrosley commented Oct 14, 2020

@AndreCimander, curious if this happened before isort updated it's own pre-commit hook to include that by default, or if you / your client was using a pre-commit mirror instead?

@AndreCimander
Copy link

@timothycrosley I was quite recently and I only found the isort pre-commit hook in the yaml, so I would guess the latter. The main issue was the differing initial behaviour of the manually triggered CLI command and the pre-commit hook, since the hook passes the filenames which in turn overwrites the normal excludes (which, imho, is the valid behaviour, just not 100% obvious).

This created some minor acceptance-grumbling within the team against pre-commit, which ensured lengthy discussions that could have been avoided ((makes one wonder if other teams also had those discussions where the argument never change a running system prevailed))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
black Black compatibility issue documentation Improvements or additions to documentation enhancement New feature or request Hacktoberfest
Projects
None yet
Development

No branches or pull requests

4 participants