Skip to content

Add missing attributes to contextlib._(Async)GeneratorContextManager #6676

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 20 commits into from
Dec 26, 2021

Conversation

AlexWaygood
Copy link
Member

No description provided.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood marked this pull request as ready for review December 23, 2021 18:05
@AlexWaygood
Copy link
Member Author

Hmm, well mypy's support of ParamSpec is still far from perfect, judging by that diff.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

I have no idea what those pytype errors mean.

…meters for `_GeneratorContextManager` has not changed, should be done in a separate PR)
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood changed the title Add contextlib._GeneratorContextManagerBase Add missing attributes to _(Async)GeneratorContextManager Dec 23, 2021
@AlexWaygood AlexWaygood changed the title Add missing attributes to _(Async)GeneratorContextManager Add missing attributes to contextlib._(Async)GeneratorContextManager Dec 23, 2021
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@rchen152
Copy link
Collaborator

I don't see any obvious issues in how _P is being used here, so the test failure is likely due to a bug in pytype - sorry about that! I'll take a look, but it'll be at least a week before a fix is released, since we're currently in a holiday freeze.

@AlexWaygood
Copy link
Member Author

I don't see any obvious issues in how _P is being used here, so the test failure is likely due to a bug in pytype - sorry about that! I'll take a look, but it'll be at least a week before a fix is released, since we're currently in a holiday freeze.

Of course, no worries!!

@srittau srittau added the status: deferred Issue or PR deferred until some precondition is fixed label Dec 24, 2021
@AlexWaygood
Copy link
Member Author

I've taken out the use of _P for now, so that this is mergeable — the more precise types can always be added later if the pytype/mypy errors are fixed 🙂

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pyinstrument (https://github.com/joerick/pyinstrument)
- pyinstrument/vendor/decorator.py:295: error: Incompatible types in assignment (expression has type "Callable[[Any, Any, VarArg(Any), KwArg(Any)], Any]", variable has type "Callable[[object], None]")
+ pyinstrument/vendor/decorator.py:295: error: Incompatible types in assignment (expression has type "Callable[[Any, Any, VarArg(Any), KwArg(Any)], Any]", variable has type "Callable[[_GeneratorContextManager[_T_co], Callable[..., Iterator[_T_co]], Tuple[Any, ...], Dict[str, Any]], None]")
- pyinstrument/vendor/decorator.py:301: error: Incompatible types in assignment (expression has type "Callable[[Any, Any, VarArg(Any), KwArg(Any)], Any]", variable has type "Callable[[object], None]")
+ pyinstrument/vendor/decorator.py:301: error: Incompatible types in assignment (expression has type "Callable[[Any, Any, VarArg(Any), KwArg(Any)], Any]", variable has type "Callable[[_GeneratorContextManager[_T_co], Callable[..., Iterator[_T_co]], Tuple[Any, ...], Dict[str, Any]], None]")

@srittau srittau removed the status: deferred Issue or PR deferred until some precondition is fixed label Dec 26, 2021
@srittau srittau merged commit 0e75580 into python:master Dec 26, 2021
@AlexWaygood AlexWaygood deleted the contextlib branch December 26, 2021 13:09
@rchen152
Copy link
Collaborator

I figured out why pytype was erroring here! The ParamSpec attribute for keyword args is called kwargs, but this PR was using kwds. The next pytype release will change pytype's error message from that weird "Can't find pyi for '_P'" to "Unrecognized ParamSpec attribute: kwds", which I think should be much clearer.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jan 1, 2022

@rchen152 oh wow, I feel like such an idiot 🤦 thanks so much for looking into it!!

AlexWaygood added a commit to AlexWaygood/typeshed that referenced this pull request Jan 1, 2022
rchen152 added a commit to google/pytype that referenced this pull request Jan 4, 2022
* Don't create multiple TypeVars for the same ParamSpec. We were creating a new
  TypeVar every time we saw a ParamSpec used in a class definition, leading to
  "Duplicate top-level identifier" errors.
* Show a more helpful error message when P.args or P.kwargs is misspelled.
  Misspelling a ParamSpec attribute previously produced a mysterious "Can't
  find pyi for <ParamSpec name>" error at the dependency loading phase.

See python/typeshed#6670 and
python/typeshed#6676.

PiperOrigin-RevId: 419457152
tungol added a commit to tungol/typeshed that referenced this pull request Dec 14, 2024
I think that python#6676 showed that Paramspec didn't work, but that
wasn't actually the fault of _GeneratorContextManagerBase.
tungol added a commit to tungol/typeshed that referenced this pull request Dec 14, 2024
I think that python#6676 showed that Paramspec didn't work, but that
wasn't actually the fault of _GeneratorContextManagerBase.
JelleZijlstra pushed a commit that referenced this pull request Dec 28, 2024
I think that #6676 showed that Paramspec didn't work, but that
wasn't actually the fault of _GeneratorContextManagerBase.
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.

3 participants