document update as inplace

[keewis]

As pointed out in #2951 I don't think we are able to deprecate the return value. If we are fine with introducing the breaking change in 0.17, I can also remove the return value here.

• closesDocument that Dataset.update defaults to being inplace. #2951 (?)
• Tests added
• Passes pre-commit run --all-files
• User visible changes (including notable bug fixes) are documented in whats-new.rst

[dcherian]

I think we should warn for a while before removing the return value.

[keewis]

how would we do that? As far as I can tell the only options are: always warn (but that would also warn if the return value is not used) or wrap in a custom object and warn in __getattr__ (but that would break isinstance checks).

[dcherian]

:embarassed: I didn't think of that. Maybe we make the breaking change in v1.0? could add a "pending deprecation" message in the docstring?

[keewis]

could add a "pending deprecation" message in the docstring?

I added the deprecation to both the docstring and whats-new.rst. This won't reach as many people as warning would, but I guess this is as much as we are able to do. Should we add a estimated release for the removal (0.19)?

[max-sixty]

LGTM!

Your call on ETA — 0.19 sounds very reasonable. Also OK to see how we go.

[max-sixty]

I was on the fence about doing the release this evening anyway — I may as well wait until we're decided and this is merged — I'll complete in the next couple of days.

[mathause]

Could we modify the original Dataset class with a special attribute? (Not sure if that'd actually work.)

classDataset(...)
_from_update=Falsedefupdate(self, other: "CoercibleMapping") ->"Dataset":
merge_result=dataset_update_method(self, other)
self._replace(inplace=True, **merge_result._asdict())
out=self.copy()
out._from_update=Truereturnoutdef__getattr__(self, name: str) ->Any:
ifself._from_update:
warnings.warn("")
self._from_update=Falsereturnsuper().__getattr__(name)

[keewis]

that could work, but we would need to use __getattribute__ instead (__getattr__ is only called if AttributeError would be raised otherwise). Not sure if the increased visibility is worth the additional complexity, though.

[keewis]

if we decide the overhead introduced by @mathause's suggestion is worth it, I can try adding that. Does not have to be in this PR (or included in the release), though.

[keewis]

let's include this in the release, even though we still have to decide if we should use the __getattributes__ method