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

adodbapi: Simplify simple functions by using assignments and fix misplaced docstrings #2250

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Apr 24, 2024

adodbapi version of #2214

Copy link
Collaborator

@vernondcole vernondcole left a comment

Choose a reason for hiding this comment

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

I'm not sure if this would fulfill the requirements of PEP249 which, if I understand correctly, requires a function here. Maybe it would work right, but feels funny and may be hard to understand.
In any case, the string in the line following the def is a docstring, and if the def is removed then the docstring should become a comment.
@mhammond might weigh in on this.

@Avasam
Copy link
Collaborator Author

Avasam commented Jun 1, 2024

As far as the docstring goes, there's still some value statically for variables, even though it doesn't end up in the runtime __doc__.
image

To keep the same __doc__ as the descriptions in https://peps.python.org/pep-0249/#type-objects-and-constructors , then these changes are not valid for PRP 249 functions.

@Avasam Avasam requested a review from vernondcole June 1, 2024 19:07
@Avasam Avasam force-pushed the adodbapi-simple-function-wrappers branch from b245208 to b445eab Compare June 1, 2024 19:09
@Avasam Avasam changed the title adodbapi: Simplify simple functions by using assignments adodbapi: Simplify simple functions by using assignments and fix misplaced docstrings Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants