GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-119127: functools.partial placeholders by dg-pb · Pull Request #119303 · python/cpython

Conversation

@dg-pb

@dg-pb dg-pb commented May 21, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

As I already had implementation I though PR might be helpful for others to see and evaluate.

From all the different extensions of functools.partial I think this one is the best. It is relatively simple and exposes all missing functionality. Other partial extensions 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 partial instances.

Performance penalty compared to current functools.partial is 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.partial so that it has flexibility of lambda / def approach (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 partial class.

Current implementation functions reliably.

  • Issue: functools.partial placeholders #119127

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
dg-pb added 2 commits May 21, 2024 13:50
@dg-pb
Initial Implementation
ee7333c
@dg-pb
serialization fix
8bcc462
@dg-pb
dg-pb requested a review from rhettinger as a code owner May 21, 2024 11:49
@bedevere-app bedevere-app Bot added the awaiting review label May 21, 2024
@dg-pb
bug fix
c67c9b4
nineteendo
nineteendo reviewed May 21, 2024
View reviewed changes
Comment thread Modules/_functoolsmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@nineteendo

nineteendo commented May 21, 2024

Copy link
Copy Markdown
Contributor

Could you add some tests? Or is that pointless without approval?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dg-pb

dg-pb commented May 21, 2024

Copy link
Copy Markdown
Contributor Author

I was thinking to wait for approval first.

nineteendo reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nineteendo

This comment was marked as outdated.

Sign in to view
@dg-pb dg-pb changed the title gn-119127: functools.partial placeholders gh-119127: functools.partial placeholders May 21, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 21, 2024
functools.partial placeholders #119127
Closed
dg-pb added 2 commits May 21, 2024 17:43
@dg-pb
Bug 2 fix
680d900
@dg-pb
Py_TPFLAGS_IMMUTABLETYPE added
9591ff5
@rhettinger rhettinger self-assigned this May 21, 2024
@rhettinger rhettinger added the DO-NOT-MERGE label May 21, 2024
dg-pb added 5 commits May 23, 2024 17:21
@dg-pb
placeholder added to state as opposed to being used as global constant
067e938
@dg-pb
static removed
8af20b3
@dg-pb
creating sentinel via PyType_Spec
607a0b1
@dg-pb
more accurate variable name
f55801e
@dg-pb
trailing trim bug and tests
5894145
@dg-pb

dg-pb commented May 24, 2024

Copy link
Copy Markdown
Contributor Author

If this was accepted, the implementation is ready for review. Python and C versions are in sync and tests implemented.

nineteendo reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dg-pb and others added 9 commits May 25, 2024 00:38
@dg-pb
Updated docs
3722e07
@dg-pb
blurb
a79c2af
d.grigonis
Merge branch 'main' into implement-119127
12aaa72
@dg-pb
minor edit
92c767b
@dg-pb
doc fix
496a9d2
@JelleZijlstra
pythongh-119180: Update the magic number (python#119397)
84be524
PR python#119321 added a comment about the magic number bump
but did not actually apply the new magic number.
@tim-one
pythongh-119105: Differ.compare is too slow [for degenerate cases] (p…
de19694
…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.
@serhiy-storchaka
pythongh-111999: Fix the signature of str.format_map() (python#119540)
08e6543
@hugovk
FAQ: Add reference to Python version numbering scheme (python#119225)
a531fd7
@erlend-aasland
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 May 31, 2024 07:39
@erlend-aasland

erlend-aasland commented May 31, 2024

Copy link
Copy Markdown
Contributor

Please open a new clean PR against main. Make sure the diff against main is correct before opening the PR.

nineteendo reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland erlend-aasland closed this May 31, 2024
@nineteendo

nineteendo commented May 31, 2024

Copy link
Copy Markdown
Contributor

Thanks Erlend.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexWaygood AlexWaygood AlexWaygood left review comments
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner
+1 more reviewer
@nineteendo nineteendo nineteendo left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@rhettinger rhettinger

Labels

awaiting review DO-NOT-MERGE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

19 participants

@dg-pb @nineteendo @erlend-aasland @manueljacob @AlexWaygood @rhettinger @JelleZijlstra @tim-one @serhiy-storchaka @hugovk @pablogsal @ericsnowcurrently @barneygale @nedbat @Wulian233 @scoder @gaogaotiantian @jaraco @xyb
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.