Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove all unused and warn-raising methods from AbstractDataStore #4310

Merged
merged 1 commit into from
Aug 5, 2020
Merged

Remove all unused and warn-raising methods from AbstractDataStore #4310

merged 1 commit into from
Aug 5, 2020

Conversation

alexamici
Copy link
Collaborator

@alexamici alexamici commented Aug 4, 2020

@shoyer this could be the first pass of the simplification of the current backend API. It not needed for #4309, but it make more obvious that we'd be free to keep the AbstractDataStore class if we wanted to.

The reason the change can be considered reasonably innocuous is that warnings would have been printed on access to any of the removed methods, but you never know. Your call (but I'd love to have a delete-only contribution to xarray :D).

Please note that:

  1. this change is the absolute lowest hanging fruit,
  2. removing __enter__ and __exit__ appears feasible as they are used only in tests, but it is a slightly bigger change,
  3. removing load is marginally more complex because it handle the special case when a variable name is equal to None.

Copy link
Member

@shoyer shoyer left a comment

Choose a reason for hiding this comment

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

This looks good to me!

I don't think we have ever recommended consuming xarray's datastore objects outside of xarray, so this seems safe.

@alexamici
Copy link
Collaborator Author

alexamici commented Aug 5, 2020

@shoyer & @jhamman you can merge the PR as it is (safest and my preferred option) or if you prefer to have also option 2. I can easily implement it in this PR by adding contextlib.closing to with-statements in the tests.

Option 3. needs a bit more discussion and I would open a different PR for that.

@max-sixty max-sixty merged commit 1101eca into pydata:master Aug 5, 2020
dcherian added a commit to rpgoldman/xarray that referenced this pull request Aug 14, 2020
* 'master' of github.com:pydata/xarray: (260 commits)
  Increase support window of all dependencies (pydata#4296)
  Implement interp for interpolating between chunks of data (dask) (pydata#4155)
  Add @mathause to current core developers. (pydata#4335)
  install sphinx-autosummary-accessors from conda-forge (pydata#4332)
  Use sphinx-accessors-autosummary (pydata#4323)
  ndrolling fixes (pydata#4329)
  DOC: fix typo argmin -> argmax in DataArray.argmax docstring (pydata#4327)
  pin sphinx to 3.1(pydata#4326)
  nd-rolling (pydata#4219)
  Implicit dask import 4164 (pydata#4318)
  allow customizing the inline repr of a duck array (pydata#4248)
  silence the known docs CI issues (pydata#4316)
  enh: fixed pydata#4302 (pydata#4315)
  Remove all unused and warn-raising methods from AbstractDataStore (pydata#4310)
  Fix map_blocks example (pydata#4305)
  Fix docstring for missing_dims argument to isel methods (pydata#4298)
  Support for PyCharm remote deployment (pydata#4299)
  Update map_blocks and map_overlap docstrings (pydata#4303)
  Lazily load resource files (pydata#4297)
  warn about the removal of the ufuncs (pydata#4268)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Aug 15, 2020
* upstream/master: (34 commits)
  Fix bug in computing means of cftime.datetime arrays (pydata#4344)
  fix some str accessor inconsistencies (pydata#4339)
  pin matplotlib in ci/requirements/doc.yml (pydata#4340)
  Clarify drop_vars return value. (pydata#4244)
  Support explicitly setting a dimension order with to_dataframe() (pydata#4333)
  Increase support window of all dependencies (pydata#4296)
  Implement interp for interpolating between chunks of data (dask) (pydata#4155)
  Add @mathause to current core developers. (pydata#4335)
  install sphinx-autosummary-accessors from conda-forge (pydata#4332)
  Use sphinx-accessors-autosummary (pydata#4323)
  ndrolling fixes (pydata#4329)
  DOC: fix typo argmin -> argmax in DataArray.argmax docstring (pydata#4327)
  pin sphinx to 3.1(pydata#4326)
  nd-rolling (pydata#4219)
  Implicit dask import 4164 (pydata#4318)
  allow customizing the inline repr of a duck array (pydata#4248)
  silence the known docs CI issues (pydata#4316)
  enh: fixed pydata#4302 (pydata#4315)
  Remove all unused and warn-raising methods from AbstractDataStore (pydata#4310)
  Fix map_blocks example (pydata#4305)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Aug 16, 2020
* upstream/master: (40 commits)
  Fix bug in computing means of cftime.datetime arrays (pydata#4344)
  fix some str accessor inconsistencies (pydata#4339)
  pin matplotlib in ci/requirements/doc.yml (pydata#4340)
  Clarify drop_vars return value. (pydata#4244)
  Support explicitly setting a dimension order with to_dataframe() (pydata#4333)
  Increase support window of all dependencies (pydata#4296)
  Implement interp for interpolating between chunks of data (dask) (pydata#4155)
  Add @mathause to current core developers. (pydata#4335)
  install sphinx-autosummary-accessors from conda-forge (pydata#4332)
  Use sphinx-accessors-autosummary (pydata#4323)
  ndrolling fixes (pydata#4329)
  DOC: fix typo argmin -> argmax in DataArray.argmax docstring (pydata#4327)
  pin sphinx to 3.1(pydata#4326)
  nd-rolling (pydata#4219)
  Implicit dask import 4164 (pydata#4318)
  allow customizing the inline repr of a duck array (pydata#4248)
  silence the known docs CI issues (pydata#4316)
  enh: fixed pydata#4302 (pydata#4315)
  Remove all unused and warn-raising methods from AbstractDataStore (pydata#4310)
  Fix map_blocks example (pydata#4305)
  ...
@alexamici alexamici deleted the simplify-AbstractDataStore branch September 22, 2020 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants