Add PyDict.update() and PyDict.update_if_missing()

[samuelcolvin]

Fix #2910

Note, I'd also be happy to remove the override_ argument from merge and perhaps rename it to update_missing or similar to give a cleaner API. LMK what you think.

Please consider adding the following to your pull request:

• an entry for this PR in newsfragments
• docs to all new functions and / or detail in the guide
• tests for all new or changed functions

[davidhewitt]

Thanks, looks fine to add this! I think override_ is fine, it matches the C API and isn't strictly speaking part of the API so much as just documentation given Rust doesn't have named arguments.

I have a couple small suggestions to tweak this, and then let's squash & merge 👍

[davidhewitt]

I think it's probably worth adding what happens if override_ is false (as I understand it, existing keys are not updated).

[davidhewitt]

It looks like other is allowed to be &PyMapping here from the CPython docs, PyDict has .as_mapping() conversion so this wouldn't make it significantly harder to use if we allowed the wider type constraint.

[mejrs]

Maybe it would be better to use an enum for this, like

pub enum Override{
    ReplaceSelf,
    ReplaceOther,
}

This gets rid of that _ too 🤮

[davidhewitt]

An enum instead of bool can help with documentation, so I'm fine with doing that. Not sure how it removes _ from the variable name?

[davidhewitt]

If you'd like the underscore gone and to use an enum, my suggestion would be to call the parameter replace_existing and then have

enum ReplaceExisting {
    No,
    Yes,
}

[samuelcolvin]

Surely we just change the variable name to replace_existing, although I think it's less clear than the current solution which shadows the cpython c API.

I also think using an enum is significantly uglier than a bool - another import, an unexpected API, and it would make me think there should be more than 2 options.

We could use over_ride, but personally I think what's here fine.

[mejrs]

Not sure how it removes _ from the variable name?

Uh yeah...not sure how I thought that

I also think using an enum is significantly uglier than a bool

My gut feeling is that this is one of those methods where people are going to mess up. I think I'd have to look up which overrides what half the time 😅 . So maybe splitting this up in multiple methods is a better idea.

[samuelcolvin]

So maybe splitting this up in multiple methods is a better idea.

Well in that case we can drop the case of merge(&self, other: &PyDict, true) since according to the C API docs, it has the same behaviour as update(other) (and presumably is slower???).

So we could then have:

• fn update(&self, other: &PyDict) - unchanged
• fn update_if_missing(&self, other: &PyDict) -> {merge(other, false)} - where existing values are not changed, somewhat similar to setdefault but for multiple values

The question then is what we call update_if_missing???

We could use update_if_missing, update_weak or just update_missing as I mentioned at the beginning.

[davidhewitt]

So I looked in the CPython source and it turns out that PyDict_Merge(x, y, 1) is exactly equivalent to PyDict_Update(x, y): https://github.com/python/cpython/blob/4246fe977d850f8b78505c982f055d33d52ff339/Objects/dictobject.c#L2932

So it's correct that there are really only two cases here. update and update_if_missing seem reasonable to me.

Final observation is that these functions support arbitrary mappings, not just Dict. So I suggest we go for:

fn update(&self, other: &PyMapping) -> PyResult<()>
fn update_if_missing(&self, other: &PyMapping) -> PyResult<()> 

If you really want &PyDict arguments, for completeness we'd need:

fn update(&self, other: &PyDict) -> PyResult<()>
fn update_if_missing(&self, other: &PyDict) -> PyResult<()> 
fn update_from_mapping(&self, other: &PyMapping) -> PyResult<()>
fn update_if_missing_from_mapping(&self, other: &PyMapping) -> PyResult<()> 

But there would be no functional difference, and PyDict::as_mapping is a zero-cost conversion.

[samuelcolvin]

• Updated to update and update_if_missing
• Switched to PyMapping
• updated docs

[davidhewitt]

LGTM, thanks! I'll force-push to squash and then merge.

[davidhewitt]

bors r+

[samuelcolvin]

Awesome, thank you.

[bors]

Build succeeded:

• conclusion