-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
collections
: Add missing reflected BinOp methods
#7207
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
Conversation
This comment has been minimized.
This comment has been minimized.
stdlib/collections/__init__.pyi
Outdated
def __mod__(self: Self, args: Any) -> Self: ... | ||
def __rmod__(self: Self, template: object) -> Self: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is actually broken:
In [12]: [] % collections.UserString("x")
Traceback (most recent call last):
File "/main_instance_shell/jelle/venv/lib/python3.6/site-packages/IPython/core/interactiveshell.py", line 3343, in run_code
exec(code_obj, self.user_global_ns, self.user_ns)
File "<ipython-input-12-0ca5ac41c5a2>", line 1, in <module>
[] % collections.UserString("x")
File "/main_instance_shell/jelle/venv/lib/python3.6/collections/__init__.py", line 1162, in __rmod__
return self.__class__(format % args)
NameError: name 'args' is not defined
It should error but not with a NameError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re the NameError
, do you wanna file the BPO issue or shall I? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go ahead! I think this method just shouldn't exist. It's presumably there to support %-formatting, but I don't think a reflected method makes sense for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, I can't reproduce the NameError
. This is what I get on my machine:
>>> from collections import UserString
>>> [] % UserString('foo')
'[]'
>>> [] % UserString('x')
'[]'
>>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it got fixed in https://bugs.python.org/issue25652. That's what I get for trying things out on 3.6. I admit I don't understand the reasoning they ended up with for keeping the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collections.UserString
does feel like it was written during a phase when Raymond Hettinger was incredibly hyped about Javascript or something:
>>> 5 + UserString('foo')
'5foo'
>>> NotImplemented + UserString('foo')
'NotImplementedfoo'
Like... wut...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In [17]: collections.UserString("x") + collections.UserList(["y"])
Out[17]: "x['y']"
In [18]: collections.UserList(["y"]) + collections.UserString("x")
Out[18]: ['y', 'x']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truly a nightmare of what Python could have been
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
This comment has been minimized.
This comment has been minimized.
The mypy-primer errors are here: https://github.com/sphinx-doc/sphinx/blob/4.x/sphinx/locale/__init__.py#L67. They're already type-ignoring similar issues with |
Diff from mypy_primer, showing the effect of this PR on open source code: sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/locale/__init__.py: note: In member "__radd__" of class "_TranslationProxy":
+ sphinx/locale/__init__.py:70:5: error: Argument 1 of "__radd__" is incompatible with supertype "UserString"; supertype defines the argument type as "object"
+ sphinx/locale/__init__.py:70:5: note: This violates the Liskov substitution principle
+ sphinx/locale/__init__.py:70:5: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
+ sphinx/locale/__init__.py:70:5: error: Return type "str" of "__radd__" incompatible with return type "_TranslationProxy" in supertype "UserString"
+ sphinx/locale/__init__.py: note: In member "__rmod__" of class "_TranslationProxy":
+ sphinx/locale/__init__.py:76:5: error: Return type "str" of "__rmod__" incompatible with return type "_TranslationProxy" in supertype "UserString"
+ sphinx/locale/__init__.py: note: In member "__rmul__" of class "_TranslationProxy":
+ sphinx/locale/__init__.py:82:5: error: Return type "str" of "__rmul__" incompatible with return type "_TranslationProxy" in supertype "UserString"
|
Testing confirms that mypy currently raises false positives for these operations, whereas they all succeed at runtime.