Skip to content

Conversation

@henryiii
Copy link
Contributor

@henryiii henryiii commented Nov 27, 2025

Alternative to #998, using __replace__. I've also dropped the matching instance if unchanged feature, since it's building up the instance as it goes, so it wouldn't really save much.

Assuming it looks good, I think we shouldn't expose .replace, and just rely on .__replace__, haven't changed the tests to that though. Edit, done.

@henryiii henryiii marked this pull request as ready for review November 27, 2025 22:10
@henryiii henryiii changed the title feat: support __replace__ for Version feat: support __replace__ for Version Nov 27, 2025
@notatallshaw
Copy link
Member

notatallshaw commented Nov 27, 2025

I'm not against using __replace__ but I'm a bit confused on the API reading the minimal documentation. Outside having an API that works against multiple generic containers, like data classes and named tuples, but is it valuable for objects like packaging.Version? For me a comparable structure would be datetime.datetime, does that implement __replace__?

Also, I don't understand how to best call it across all Python versions that don't support it, e.g. in #999 would I directly call __replace__ or have to some logic like in your tests?

Finally, does the fact the API is related to copy imply it must return a copy even if no attributes have changed?

@henryiii
Copy link
Contributor Author

henryiii commented Nov 27, 2025

Outside having an API

It's a standard API, so someone reading copy.replace(version, release=(2,3,4)) would know what it's supposed to do. __replace__ has a specific meaning, and that's what we are trying to add, so it seems like the right thing to do, rather than adding more API. You don't have to look up docs for __replace__, but you do for any custom API added. .replace might do an in-place replacement, for example.

Would I directly call __replace__

Sure. I mostly wanted tests to verify copy.replace was working on 3.13+, otherwise I'd have done it in the tests too.

Imply it must return a copy

I'm nearly sure the answer is no, if an object is immutable (which Version is supposed to be), then it's an implementation detail if it's copied or not. It just did not save anything for me to return if unchanged unless nothing was passed, which I assume isn't really useful. I could keep track of if a change was made then return the original if no change was made, but that adds extra comparisons on every step, for I would assume very little gain (maybe the memory savings would be worth it, though?)

It does imply that the original object is not modified.

@henryiii henryiii marked this pull request as draft November 28, 2025 02:47
@henryiii
Copy link
Contributor Author

henryiii commented Nov 28, 2025

I think I can clean this up a bit, and I want to see if I can get the no-copy feature back in, so putting it in draft for a bit. I need some sort of performance test too.

    Co-authored-by: Damian Shaw <[email protected]>

Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii force-pushed the henryiii/feat/replace branch from 6863f73 to eab10c1 Compare November 28, 2025 15:04
@henryiii
Copy link
Contributor Author

Okay, I've restored the "no copy if unchanged" feature, and cleaned it up.

@henryiii henryiii marked this pull request as ready for review November 28, 2025 15:07
Copy link
Member

@notatallshaw notatallshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@henryiii henryiii merged commit f88ea3c into pypa:main Nov 29, 2025
40 checks passed
@henryiii henryiii deleted the henryiii/feat/replace branch November 29, 2025 21:11
This was referenced Nov 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants