gh-119127: functools.partial placeholders#119303
Closed
Uh oh!
There was an error while loading. Please reload this page.
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
Uh oh!
There was an error while loading. Please reload this page.
Contributor
nineteendo commented May 21, 2024
Could you add some tests? Or is that pointless without approval? |
ContributorAuthor
dg-pb commented May 21, 2024
I was thinking to wait for approval first. |
This comment was marked as outdated.
This comment was marked as outdated.
dg-pb changed the title 
ContributorAuthor
dg-pb commented May 24, 2024
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.
erlend-aasland removed request for a team, Fidget-Spinner, abalkin, ambv, barneygale, brettcannon, corona10, ericsnowcurrently, ericvsmith, erlend-aasland, ethanfurman, ezio-melotti, freakboy3742, gpshead, hugovk, iritkatriel, isidentical, jaraco, kumaraditya303, markshannon, ncoghlan, pganssle, serhiy-storchaka, tiran, vsajip and warsaw
Contributor
erlend-aasland commented May 31, 2024
Please open a new clean PR against main. Make sure the diff against main is correct before opening the PR. |
Contributor
nineteendo commented May 31, 2024
Thanks Erlend. |
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
19 participants
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.