Skip to content

Conversation

@clot27
Copy link
Member

@clot27 clot27 commented Sep 29, 2023

closes #3886
I wonder if we should add docstring for args in __aexit__?

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docstring nitpicks mostly:

@harshil21 harshil21 added the ⚙️ documentation affected functionality: documentation label Sep 29, 2023
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR! In addition to my below comments:

  • I see no added value in documenting the arguments of __aexit__, as they are defined by Python itself. If you like to, you can add something like Please refer to the [Python documentation](https://docs.python.org/3.11/reference/datamodel.html?highlight=__aexit__#object.__aexit__) for information about the accepted arguments
  • We have these parts "Instances of this class can be used as asyncio context managers, where …" in the class docstrings of the affected classes. Could you add a "seealso aenter & exit" below these?

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! :)

@Bibo-Joshi Bibo-Joshi merged commit af130ef into master Oct 9, 2023
@Bibo-Joshi Bibo-Joshi deleted the add-ctxmanager-docstring branch October 9, 2023 16:59
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

⚙️ documentation affected functionality: documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Documentation]: Context manager dunder Methods

4 participants