deprecate encoding setters #7708
Conversation
github-actions
bot
added
io
topic-backends
topic-CF conventions
topic-dask
topic-zarr
fix bad merge
jhamman
force-pushed
the
dep/encoding-setter
branch
from
6cfd646 to
988dfcf
Compare
til - property setters need to directly follow the property def
There was a problem hiding this comment.
This is ready for a quick look from folks. Tests are passing and we're not leaking FutureWarnings on internal calls anymore.
Something I could use some feedback on is how we want to handle encoding.__setitem__ warnings. As implmented now, this warns:
ds.encoding = {"unlimited_dims": {"x"}}but this does not:
ds["unlimited_dims"] = {"x"}To issue a warning from the latter, we need to override the __setitem__ on the _encoding attribute which is currently a vanilla dict. I'm slightly concerned about going down this path but I think it is possible if we are willing to change the type of the encoding attribute.
| @@ -512,6 +512,7 @@ def test_roundtrip_string_data(self) -> None: | |||
| with self.roundtrip(expected) as actual: | |||
| assert_identical(expected, actual) | |||
|
|
|||
| @pytest.mark.filterwarnings("ignore:Setting encoding directly.*:FutureWarning") | |||
There was a problem hiding this comment.
I've explicitly marked all tests that use the encoding setter with this filter. When we remove the setter, we'll need to come back through and rework these tests.
| @encoding.setter | ||
| def encoding(self, value: Mapping[Any, Any]) -> None: | ||
| # deprecated (variable setter will warn) | ||
| self.variable.encoding = dict(value) | ||
|
|
||
| def _set_encoding_internal(self, value: Mapping[Any, Any]) -> None: | ||
| """temporary method to set encoding without issuing a FutureWarning""" | ||
| self.variable._set_encoding_internal(value) |
There was a problem hiding this comment.
In the future when @encoding.setter is gone, we will likely still wan/need a way to insert encoding attributes onto objects (coding and backends currently require this). I'm open to new names but this method is intended to be that code path.
encoding#6323whats-new.rstapi.rst