gh-119127: functools.partial placeholders #119303
Closed
Conversation
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
nineteendo
reviewed
May 21, 2024
|
Could you add some tests? Or is that pointless without approval? |
|
I was thinking to wait for approval first. |
This comment was marked as outdated.
This comment was marked as outdated.
|
If this was accepted, the implementation is ready for review. Python and C versions are in sync and tests implemented. |
PR python#119321 added a comment about the magic number bump but did not actually apply the new magic number.
…ython#119492) ``_fancy_replace()`` is no longer recursive. and a single call does a worst-case linear number of ratio() computations instead of quadratic. This renders toothless a universe of pathological cases. Some inputs may produce different output, but that's rare, and I didn't find a case where the final diff appeared to be of materially worse quality. To the contrary, by refusing to even consider synching on lines "far apart", there was more easy-to-digest locality in the output.
|
Please open a new clean PR against main. Make sure the diff against main is correct before opening the PR. |
|
Thanks Erlend. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As I already had implementation I though PR might be helpful for others to see and evaluate.
From all the different extensions of
functools.partialI think this one is the best. It is relatively simple and exposes all missing functionality. Otherpartialextensions that I have seen lack functionality and would not provide complete argument ordering capabilities and/or are too complicated in relation to what they offer.Implementation can be summarised as follows:
a) All trailing placeholders are removed. (Makes things simpler)
b) Throws exception if not all placeholders are filled on call
c) retains optimization benefits of application on other
partialinstances.Performance penalty compared to current
functools.partialis minimal for extension class. + 20-30 ns for initialisation and <4 ns when called with or without placeholders.To put it simply, new functionality extends
functools.partialso that it has flexibility oflambda/defapproach (in terms of argument ordering), but call overhead is 2x smaller.The way I see it is that this could only be justified if this extension provided completeness and no new functionality is going to be needed anywhere near in the future. I have thought about it and tried various alternatives and I think there is a good chance that this is the case. Personally, I don't think I would ever need anything more from
partialclass.Current implementation functions reliably.