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

follow python typing export requirements #3032

Closed
wants to merge 5 commits into from
Closed

follow python typing export requirements #3032

wants to merge 5 commits into from

Conversation

danpf
Copy link

@danpf danpf commented Apr 24, 2022

Description

Black's __init__.py file does not explicitly alias imported modules as themselves which is required for type-checking.

I grepped the code for things that were directly called from black. and added X as X for anything mentioned in the code + anything I thought seemed related.

This isn't really very important, I was just annoyed by some of the pyright warnings I was getting when using black.Mode.

Happy to alter the PR in anyway, if you think more or less should be exported from __init__.py

https://github.com/python/typing/blob/master/docs/source/stubs.rst#imports

I don't think it needs a mention in the changelog.

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@github-actions
Copy link

github-actions bot commented Apr 24, 2022

diff-shades results comparing this PR (c528bb6) to main (fb8dfde). The full diff is available in the logs under the "Generate HTML diff report" step.

╭─────────────────────── Summary ────────────────────────╮
│ 1 projects & 1 files changed / 0 changes [+0/-0]       │
│                                                        │
│ ... out of 2 203 867 lines, 10 569 files & 23 projects │
╰────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

@Jackenmen
Copy link
Contributor

Jackenmen commented Apr 24, 2022

I would say that __all__ would be the preferred option here (and is supported by both type checkers and various linters which is not necessarily the case with X as X syntax).

The X as X makes more sense in stub files where __all__ should only be defined if the real package actually defines it.
Here's more information: https://typing.readthedocs.io/en/latest/source/libraries.html#library-interface

@JelleZijlstra
Copy link
Collaborator

I'm a bit hesitant about this, because at the moment Black is only an application, not a library. We have a longstanding issue about defining a public API, but I hesitate to mark anything as explicitly part of the API until we finish that issue.

@danpf
Copy link
Author

danpf commented Apr 25, 2022

would say that all would be the preferred option here

Can change it if you'd like. I only prefer this way because I find it's easier to maintain. I'm not attached.

I'm a bit hesitant about this

That's fine, if you don't think it's a good fit for the library or the library-as-it-is-now feel free to close the issue. no hard feelings.

I guess to combine the two comments:

Even after reading the documentation/best-practices/SO-posts w/r/t this area I find it all slightly murky tbh so I'll do whatever you both suggest. I have personally decided that __all__ = is for hard exports that are integral to an API (changes require major version bump), while I use X as X for things that are more fluid but convenient at the time.

Under this logic I make sure to never version anything I develop past beta so I don't have to use __all__ :p

@felix-hilden
Copy link
Collaborator

Quick question, since this is new for me: looking at the link you provided, I see only a mention of stub files (it being a doc for stub files, duh). But this PR would apply it to our source. So does the same aliasing distinction indeed apply to ordinary sources as well?

I'd gravitate towards __all__ as well, or even cleaning up the top level namespace to only include the things that we want to export in the first place. But let's discuss that with the public API.

@danpf
Copy link
Author

danpf commented May 4, 2022

So does the same aliasing distinction indeed apply to ordinary sources as well?

I can't answer your question without any doubt, but in my experience since inline typing has increased in popularity the line between stub files and actual python code has gotten increasingly blurry.

... 20 minutes later ...


From what I can tell, it appears that the current intent is: if a library has a py.typed file, the .py files can be treated as .pyi files and can be held to a higher typing standard. This interpretation seems to be mostly spread by the pyright developer Eric Traut, who has discussed (or at least mentioned) it with the powers that be. (Sorry for spreading missinformation if that's not true : o )

Both pyright and mypy seem to somewhat follow this interpretation, and since mypy is the basis of the python typing system, I think it's fair to just do what they do.

Relevant literature:

https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#library-interface
https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-no-implicit-reexport
python/mypy#8754
pytest-dev/pytest#7593
https://mail.python.org/archives/list/typing-sig@python.org/thread/YLJPWECBNPD2K4TRIBRIPISNUZJCRREY/#C2ZZKEM3EESWMDUF3PYZB4ZG2L42XUZT

not really sure where that leaves us today, but thats the skinny.


To revisit my initial statement: the lines between .py and .pyi files are still somewhat blurry, but Eric seems to be spearheading the effort to put rules in writing and de-blur the .py/.pyi file lines -- which I think is a good thing : )

@danpf danpf closed this May 13, 2022
@ichard26
Copy link
Collaborator

I know this PR was (implicitly) rejected, but I wanted to say thank you to you @danpf for opening this PR anyway. We now know to take this into account when we come around to defining a stable API for black.

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.

5 participants