Skip to content

Use "all +=" instead of duplicating the branches #7865

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

Merged
merged 10 commits into from
Jun 7, 2022

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented May 18, 2022

Also, order the items alphabetically.

@srittau
Copy link
Collaborator Author

srittau commented May 18, 2022

There have been a few warnings already when testing locally. Let's see what CI says.

@AlexWaygood
Copy link
Member

I did some manual testing of this a while back -- I believe mypy also does not support this in a stub file, even though the CI is green. IIRC, it doesn't recognise any objects from typing as being actually imported in a from typing import * statements if we define __all__ like this.

@github-actions

This comment has been minimized.

@srittau
Copy link
Collaborator Author

srittau commented May 18, 2022

I added a test case that passes, but that may be because of the special casing of typing. I'm trying another module.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

AlexWaygood commented May 18, 2022

I added a test case that passes, but that may be because of the special casing of typing. I'm trying another module.

Right, I misremembered. I think I was trying to do something more ambitious in asyncio — I was trying to do something like this:

if sys.version_info >= (3, 7):
    __all__ += runners.__all__

That's not supported by mypy IIRC.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented May 18, 2022

If this works, it would allow us to reasonably define os.__all__ in the stubs. It would expose os._exit, which type checkers currently treat as private even though it's documented.

@AlexWaygood
Copy link
Member

Looks like it also breaks pytype, though.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented May 18, 2022

I'm curious if pytype views __all__.append/extend more favourably, since that is less of a "definition" than += and we might hit a different code path

@AlexWaygood
Copy link
Member

PyCQA/flake8-pyi#219 for the flake8-pyi error

@srittau
Copy link
Collaborator Author

srittau commented May 19, 2022

@erictraut It seems that pyright doesn't accept the advanced __all__ constructs in stubs yet. Is that something you would consider changing?

@github-actions

This comment has been minimized.

@erictraut
Copy link
Contributor

erictraut commented May 19, 2022

@srittau, the __all__ constructs work fine in stubs, but the reportInvalidStubStatement is being triggered here. Yeah, I can change the logic in reportInvalidStubStatement to accept the statement forms that manipulate __all__.

I've filed this issue. Should be easy to fix. I'll probably publish the next version of pyright on Fri or Sat at the latest. If you can wait until then, great. If you want to move forward with this PR in the meantime, you could temporarily disable the reportInvalidStubStatement rule in pyrightconfig.json.

@srittau
Copy link
Collaborator Author

srittau commented May 19, 2022

I've filed microsoft/pyright#3485. Should be easy to fix. I'll probably publish the next version of pyright on Fri or Sat at the latest. If you can wait until then, great. If you want to move forward with this PR in the meantime, you could temporarily disable the reportInvalidStubStatement rule in pyrightconfig.json.

Thanks! We are not in a rush with this PR and there are a few more problems that we need to take a look at.

AlexWaygood added a commit to PyCQA/flake8-pyi that referenced this pull request May 19, 2022
AlexWaygood added a commit that referenced this pull request May 19, 2022
@srittau
Copy link
Collaborator Author

srittau commented May 19, 2022

Also Cc @rchen152: Is all += ... something that pytype could support reasonably soon in stubs?

@github-actions

This comment has been minimized.

@rchen152
Copy link
Collaborator

Also Cc @rchen152: Is all += ... something that pytype could support reasonably soon in stubs?

I'll open a pytype issue for it. (I'm about to head out of town for two weeks, so I'll look into this when I get back if none of the other devs have resolved it in the meantime.)

@erictraut
Copy link
Contributor

I've address the issue in pyright. The fix will be included in version 1.1.248, which I will publish within the next 48 hours.

@erictraut
Copy link
Contributor

I've published pyright 1.1.248, which includes this fix.

srittau added a commit to srittau/typeshed that referenced this pull request May 21, 2022
@srittau srittau mentioned this pull request May 21, 2022
AlexWaygood pushed a commit that referenced this pull request May 21, 2022
@AlexWaygood
Copy link
Member

I've published pyright 1.1.248, which includes this fix.

Pyright tests are now passing — thanks, as always, for the extremely quick fix!

@github-actions

This comment has been minimized.

AlexWaygood added a commit that referenced this pull request Jun 7, 2022
@AlexWaygood AlexWaygood mentioned this pull request Jun 7, 2022
AlexWaygood added a commit that referenced this pull request Jun 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood AlexWaygood marked this pull request as ready for review June 7, 2022 11:21
@srittau srittau merged commit 214bf15 into python:master Jun 7, 2022
@srittau srittau deleted the typing-all branch June 7, 2022 11:32
@srittau
Copy link
Collaborator Author

srittau commented Jun 7, 2022

Thanks everyone for the support!

@tungol tungol mentioned this pull request Nov 18, 2024
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.

6 participants