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

Convert parameters popped from **kwargs to keyword-only #10025

Closed
twisted-trac opened this issue Oct 17, 2020 · 4 comments
Closed

Convert parameters popped from **kwargs to keyword-only #10025

twisted-trac opened this issue Oct 17, 2020 · 4 comments

Comments

@twisted-trac
Copy link

mthuurne's avatar @mthuurne reported
Trac ID trac#10025
Type enhancement
Created 2020-10-17 05:41:40Z

While working on #10021, I found several functions/methods in Twisted which document parameters with @param but implement them by popping them from **kwargs.

While it would be possible to fix the docstrings, it may be better to convert these to keyword-only parameters. The main advantage is that having separate parameters means each parameter can receive an individual type annotation.

Another advantage is that for functions/methods that support a fixed set of keyword parameters, we can remove the **kargs. That way, passing a non-existing parameter triggers an error instead of being silently ignored, which helps catching bugs.

Searchable metadata
trac-id__10025 10025
type__enhancement enhancement
reporter__mthuurne mthuurne
priority__normal normal
milestone__None None
branch__ 
branch_author__ 
status__closed closed
resolution__fixed fixed
component__core core
keywords__review review
time__1602913300340707 1602913300340707
changetime__1603280694019765 1603280694019765
version__None None
owner__Maarten_ter_Huurne__maarten_____ Maarten ter Huurne <maarten@...>

@twisted-trac
Copy link
Author

mthuurne's avatar @mthuurne commented

This is the list flagged by pydoctor:

src/twisted/enterprise/adbapi.py:176: documented parameter "cp_min" does not exist
src/twisted/enterprise/adbapi.py:178: documented parameter "cp_max" does not exist
src/twisted/enterprise/adbapi.py:180: documented parameter "cp_noisy" does not exist
src/twisted/enterprise/adbapi.py:183: documented parameter "cp_openfun" does not exist
src/twisted/enterprise/adbapi.py:188: documented parameter "cp_reconnect" does not exist
src/twisted/enterprise/adbapi.py:193: documented parameter "cp_good_sql" does not exist
src/twisted/enterprise/adbapi.py:196: documented parameter "cp_reactor" does not exist
src/twisted/internet/endpoints.py:1988: documented parameter "caCertsDir" does not exist
src/twisted/internet/endpoints.py:1996: documented parameter "hostname" does not exist
src/twisted/internet/_sslverify.py:1221: Documented parameter "extraCertificateOptions" does not exist
src/twisted/mail/imap4.py:3586: documented parameter "uid" does not exist
src/twisted/protocols/amp.py:2117: documented parameter "tls_localCertificate" does not exist
src/twisted/protocols/amp.py:2121: documented parameter "tls_verifyAuthorities" does not exist

I haven't investigated most of these yet, beyond confirming that while they're not ordinary parameters, they can be passed as keyword arguments. So it's possible that some of them will be addressed by using @keyword rather than converting them.

@twisted-trac
Copy link
Author

mthuurne's avatar @mthuurne commented

I decided to fix the docstrings using @keyword in #10021, since it was a small effort and an improvement over the old situation. However, I still think that converting them to keyword-only parameters is better, so I'm leaving this ticket open.

@twisted-trac
Copy link
Author

mthuurne's avatar @mthuurne commented

PR is here: #1456

@twisted-trac
Copy link
Author

mthuurne's avatar @mthuurne set owner to @mthuurne
@mthuurne set status to closed

In changeset d17f169

#!CommitTicketReference repository="" revision="d17f16964ef7031d97bcda73cd50f7cd94f5a3b3"
Merge pull request #1456 from twisted/10025-mthuurne-keywords-to-params

Author: mthuurne
Reviewer: graingert
Fixes: ticket:10025
    
Convert parameters popped from **kwargs to keyword-only

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant