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

Let pre-commit sort pxd files too #1549

Merged
merged 5 commits into from Oct 12, 2020
Merged

Conversation

MarcoGorelli
Copy link
Contributor

In #1494, it looks like support for .pxd files was added. However, these are currently ignored when running isort in pre-commit because there is types: [python]

MarcoGorelli and others added 3 commits October 8, 2020 13:42
In PyCQA#1494, it looks like support for `.pxd` files was added. However, these are currently ignored when running isort in pre-commit because there is `types: [python]`
Use types, not extensions
@MarcoGorelli
Copy link
Contributor Author

Hi @timothycrosley - my understanding of types is that the intersection of them is taken, so if you have types: [python, cython, pyi] then the result will be that nothing will get sorted.

From the pre-commit docs

types and files are evaluated with AND when filtering. Tags within types are also evaluated using AND.

@timothycrosley
Copy link
Member

Yep! I came to the same conclusion myself as well. This is really unfortunate however, because it makes it impossible for isort to add seamless support for Cython files without introducing a regression on non-extensioned Python files. Looks like there is an open issue to add Or type support: pre-commit/pre-commit#607

@MarcoGorelli
Copy link
Contributor Author

MarcoGorelli commented Oct 9, 2020

Ah, I didn't think that was possible, sorry!

$ echo '#!/usr/bin/python' > nonext
$ sudo chmod +x nonext 
$ identify-cli nonext 
["executable", "file", "python", "text"]

Looks like we'll have to wait for pre-commit/pre-commit#607

@MarcoGorelli
Copy link
Contributor Author

Would it work for you to just have types: [files] until pre-commit/pre-commit#607 is in, and then after that types_or: [python, cython, pyi]?

@timothycrosley
Copy link
Member

@MarcoGorelli,

Thanks for highlighting this area of improvement!

Would it work for you to just have types: [files] until pre-commit/pre-commit#607 is in, and then after that types_or: [python, cython, pyi]?

The only problem with that is than it will cause a performance regression and maybe even behave in unexpected ways / break non-Python files. That said I would be amenable to that change if we could find some larger users of isort with pre-commit and then create an additional integration test suite that ran pre-commit against those users repositories and could confirm it didn't cause any issues. I think this would be a long-term win for isorts maintainability overall as well.

Void of that, I think the most prudent thing to do would be to update the documentation of pre-commit usage to such using a hook per a file-type as shown in this thread: pre-commit/pre-commit#1436 (comment)

Example:

- hooks:
      - id: isort
        types: [python]
      - id: isort
        types: [cython]
      - id: isort
        types: [pyi]

Thanks!

~Timothy

@MarcoGorelli
Copy link
Contributor Author

That's a good idea, have updated accordingly - thanks for this and for your awesome work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants