GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-73536: Add support for multi-signatures by serhiy-storchaka · Pull Request #117671 · python/cpython Conversation
|
|
|
||
|
|
||
| def _multisignature_get_partial(wrapped_sig, partial, extra_args=()): | ||
| last_exc = None |
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Unless the case of the list being empty is handled elsewhere, this could result in a raise None. Maybe that None should instead be something like Exception("Empty MultiSignature list") ?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It is checked in the constructor. I think even about removing this assignment, NameError is clearer indication if something goes wrong.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I agree.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| return True | ||
| if isinstance(other, MultiSignature): | ||
| return self._signatures == other._signatures | ||
| if len(self._signatures) == 1 and isinstance(other, Signature): |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think this should be
| if len(self._signatures) == 1 and isinstance(other, Signature): | |
| if isinstance(other, Signature): | |
| return len(self._signatures) == 1 |
and the addition to Signature's eq should be removed, above.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
No, it is not correct, it would make a 1-element MultiSignature equal to any Signature.
You perhaps mistyped, I wrote more correct fix.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I did. That's good. (I would suggest removing the line about MultiSignature in Signature's eq as I believe it's dead code now, but up to you).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It is not dead. It is called in SignatureSubclass() == MultiSignature(). Although it is not covered by tests yet.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| def signature(obj, *, follow_wrapped=True, globals=None, locals=None, eval_str=False): | ||
| """Get a signature object for the passed callable.""" | ||
| return Signature.from_callable(obj, follow_wrapped=follow_wrapped, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Given that the original Signature.from_callable is not changed and does the exact same call to _from_callable, that means both signature and Signature.from_callable may return a MultiSignature object, right ?
Strictly speaking this is not a violation of backwards compatibility as MultiSignature is a subclass of Signature, but I find it weird and it may break some strict-type behaviors. Is there a way to have those only return pure Signature objects, without breaking everything this update brings to the table ?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is. The idea was that inspect.signature() will start to support previously not supported cases and return a MultiSignature which should be compatible with Signature. But now I am no longer sure. They are not so well compatible as I expected, and in most cases I needed to rewrite the code to explicitly handle MultiSignature. This is why I added inspect.signatures(). Now the code is in the intermediate state -- either MultiSignature will be made more compatible with Signature and inspect.signatures() may disappear, or they will became completely different things.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
|
||
|
|
||
| def _multisignature_get_partial(wrapped_sig, partial, extra_args=()): | ||
| last_exc = None |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It is checked in the constructor. I think even about removing this assignment, NameError is clearer indication if something goes wrong.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| return True | ||
| if isinstance(other, MultiSignature): | ||
| return self._signatures == other._signatures | ||
| if len(self._signatures) == 1 and isinstance(other, Signature): |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
No, it is not correct, it would make a 1-element MultiSignature equal to any Signature.
You perhaps mistyped, I wrote more correct fix.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| def signature(obj, *, follow_wrapped=True, globals=None, locals=None, eval_str=False): | ||
| """Get a signature object for the passed callable.""" | ||
| return Signature.from_callable(obj, follow_wrapped=follow_wrapped, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is. The idea was that inspect.signature() will start to support previously not supported cases and return a MultiSignature which should be compatible with Signature. But now I am no longer sure. They are not so well compatible as I expected, and in most cases I needed to rewrite the code to explicitly handle MultiSignature. This is why I added inspect.signatures(). Now the code is in the intermediate state -- either MultiSignature will be made more compatible with Signature and inspect.signatures() may disappear, or they will became completely different things.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@serhiy-storchaka, now this again has conflicts with the main. I did pr serhiy-storchaka#22 that address this, hope it helps. Are you thinking about writing a PEP as Petr suggested? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@serhiy-storchaka, will you be ok if I continue this work in a separate pr? Or you disappointed in this approach? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Version-specific patches adapted from existing ones: - relocatable-python-config: same change as 3.x-0001 but adjusted for line offset in 3.13+ python-config.sh.in (shared between 3.13 and 3.14 since the file is identical) - remove-module-deps: removes ProjectDependencies from the python project in PCbuild/pcbuild.sln (separate patches for 3.13 and 3.14 due to different GUID sets) - _ctypes-ffi (3.13 only): converts static formattable[] array to dynamic initialization for shared libffi compatibility. Not needed for 3.14 where this was fixed upstream by restructuring cfield.c into a named struct (python/cpython#117671) The _msi-vcxproj patch is no longer needed since msilib was removed in 3.13 per PEP 594 (python/cpython#104911).
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.