FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Remove `ConverterKeywordDict` alias in `clinic.py` by sobolevn · Pull Request #115843 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

Remove ConverterKeywordDict alias in clinic.py - #115843

Merged
AlexWaygood merged 1 commit into
python:mainfrom
sobolevn:clinic-ConverterKeywordDict
Feb 23, 2024
ConversationCommits (1)ChecksFiles changed
Merged

Remove ConverterKeywordDict alias in clinic.py#115843
AlexWaygood merged 1 commit into
python:mainfrom
sobolevn:clinic-ConverterKeywordDict

Conversation

sobolevn commented Feb 23, 2024

Copy link
Copy Markdown
Member

Context: python/mypy#16939 (comment)

New mypy version detected this typing problem: dict[str, TypeSet | bool] is not the correct type for **kwargs of str_converter.__init__:

CPython (Argument Clinic) (https://github.com/python/cpython)
+ Tools/clinic/clinic.py:4094: error: Argument 1 to "str_converter" has incompatible type "**dict[str, set[type[object]] | bool]"; expected "str"  [arg-type]
+ Tools/clinic/clinic.py:4094: error: Argument 1 to "str_converter" has incompatible type "**dict[str, set[type[object]] | bool]"; expected "Function"  [arg-type]
+ Tools/clinic/clinic.py:4094: error: Argument 1 to "str_converter" has incompatible type "**dict[str, set[type[object]] | bool]"; expected "str | None"  [arg-type]
+ Tools/clinic/clinic.py:4094: error: Argument 1 to "str_converter" has incompatible type "**dict[str, set[type[object]] | bool]"; expected "str | Literal[Sentinels.unspecified]"  [arg-type]
+ Tools/clinic/clinic.py:4094: error: Argument 1 to "str_converter" has incompatible type "**dict[str, set[type[object]] | bool]"; expected "bool"  [arg-type]

(the error message looks cryptic).

str_converter.__init__ has this type:

def __init__(self,
             # Positional args:
             name: str,
             py_name: str,
             function: Function,
             default: object = unspecified,
             *,  # Keyword only args:
             c_default: str | None = None,
             py_default: str | None = None,
             annotation: str | Literal[Sentinels.unspecified] = unspecified,
             unused: bool = False,
             **kwargs: Any
    ) -> None:

So, basically you can only say that **kwargs can be dict[str, Any].
This is why I removed this alias.

sobolevn force-pushed the clinic-ConverterKeywordDict branch from 5d8959f to a59ca06 Compare February 23, 2024 08:28
AlexWaygood reviewed Feb 23, 2024
View reviewed changes

AlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Or perhaps we could do something like this? It would be slightly less dynamic, meaning mypy would have a higher chance of understanding what we're doing here:

--- a/Tools/clinic/clinic.py
+++ b/Tools/clinic/clinic.py
@@ -4069,8 +4069,6 @@ def parse_arg(self, argname: str, displayname: str, *, limited_capi: bool) -> st
 # mapping from arguments to format unit *and* registers the
 # legacy C converter for that format unit.
 #
-ConverterKeywordDict = dict[str, TypeSet | bool]
-
 def r(format_unit: str,
       *,
       accept: TypeSet,
@@ -4086,12 +4084,10 @@ def r(format_unit: str,
         #
         # also don't add the converter for 's' because
         # the metaclass for CConverter adds it for us.
-        kwargs: ConverterKeywordDict = {}
         if accept != {str}:
-            kwargs['accept'] = accept
-        if zeroes:
-            kwargs['zeroes'] = True
-        added_f = functools.partial(str_converter, **kwargs)
+            added_f = functools.partial(str_converter, accept=accept)
+        else:
+            added_f = functools.partial(str_converter, zeroes=True)
         legacy_converters[format_unit] = added_f
 
     d = str_converter_argument_map

sobolevn commented Feb 23, 2024

Copy link
Copy Markdown
Member Author

@AlexWaygood indeed! This is a better way! 👍

sobolevn commented Feb 23, 2024

Copy link
Copy Markdown
Member Author

except, it is a bit more complex. There might be two conditions at once:

  • accept != {str}
  • zeroes

AlexWaygood commented Feb 23, 2024

Copy link
Copy Markdown
Member

except, it is a bit more complex. There might be two conditions at once:

* `accept != {str}`

* `zeroes`

Ah shoot, yes, it's not a 1:1 rewrite, is it. The original doesn't use else:. Let's go with your way for now then :)

AlexWaygood approved these changes Feb 23, 2024
View reviewed changes

AlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks!

AlexWaygood enabled auto-merge (squash) February 23, 2024 08:38
AlexWaygood merged commit e3f462c into python:main Feb 23, 2024
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
Remove ConverterKeywordDict alias in clinic.py (python#115843)
e434429
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
Remove ConverterKeywordDict alias in clinic.py (python#115843)
5f4abeb
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
Remove ConverterKeywordDict alias in clinic.py (python#115843)
47948b6
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

hauntsaninja hauntsaninja approved these changes
AlexWaygood AlexWaygood approved these changes
erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

Assignees

No one assigned

Labels

skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL