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

fix: remove Coordinate from __all__ in xarray/__init__.py #8791

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

hassec
Copy link
Contributor

@hassec hassec commented Feb 28, 2024

Coordinate is in __all__ but isn't imported.
Thus a

from xarray import *

raises an error.

IMHO, this should have been caught by ruff, but ruff doesn't catch undefined entries in __all__, see astral-sh/ruff#10095
I suggest to watch that issue for resolution to potentially adjust the ruff config of this project such that these things will be caught in the future.

Copy link

welcome bot commented Feb 28, 2024

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

@max-sixty
Copy link
Collaborator

Great, thanks @hassec !

Would a test that runs from xarray import * have caught this? Would you be up for adding one?

@max-sixty
Copy link
Collaborator

max-sixty commented Feb 28, 2024

(dask tests are unrelated)

@hassec
Copy link
Contributor Author

hassec commented Feb 28, 2024

I did consider it, but star imports can't be used inside functions.
So the next best thing would be:

def test_star_import():
    import xarray
    from xarray import __all__

    xarray_dir = dir(xarray)
    for name in (__all__):
        assert name in xarray_dir, "Ensure __all__ only contains names of imported objects"

But this is only for the xarray/__init__.py so if you would want to check all __init__.py files one would have to determine all of these and then import their modules to check etc.

And at that point I was like maybe I'll just wait a few days to see if they fix ruff 😅
Note that pyright also catches this error but it seems that that isn't enabled by default in the ci?

@max-sixty
Copy link
Collaborator

And at that point I was like maybe I'll just wait a few days to see if they fix ruff 😅

OK yes, that seems very reasonable!

Note that pyright also catches this error but it seems that that isn't enabled by default in the ci?

Yeah, lots of false-ish positives at the moment...

@max-sixty max-sixty enabled auto-merge (squash) February 28, 2024 02:54
@max-sixty max-sixty disabled auto-merge February 28, 2024 02:54
@max-sixty max-sixty merged commit a241845 into pydata:main Feb 28, 2024
18 of 29 checks passed
Copy link

welcome bot commented Feb 28, 2024

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again! celebration gif

@max-sixty
Copy link
Collaborator

Thanks @hassec !

dcherian added a commit to dcherian/xarray that referenced this pull request Mar 15, 2024
* main: (31 commits)
  correctly encode/decode _FillValues/missing_values/dtypes for packed data (pydata#8713)
  Expand use of `.oindex` and `.vindex` (pydata#8790)
  Return a dataclass from Grouper.factorize (pydata#8777)
  [skip-ci] Fix upstream-dev env (pydata#8839)
  Add dask-expr for windows envs (pydata#8837)
  [skip-ci] Add dask-expr dependency to doc.yml (pydata#8835)
  Add `dask-expr` to environment-3.12.yml (pydata#8827)
  Make list_chunkmanagers more resilient to broken entrypoints (pydata#8736)
  Do not attempt to broadcast when global option ``arithmetic_broadcast=False`` (pydata#8784)
  try to get the `upstream-dev` CI to complete again (pydata#8823)
  Bump the actions group with 1 update (pydata#8818)
  Update documentation for clarity (pydata#8817)
  DOC: link to zarr.convenience.consolidate_metadata (pydata#8816)
  Refactor Grouper objects (pydata#8776)
  Grouper object design doc (pydata#8510)
  Bump the actions group with 2 updates (pydata#8804)
  tokenize() should ignore difference between None and {} attrs (pydata#8797)
  fix: remove Coordinate from __all__ in xarray/__init__.py (pydata#8791)
  Fix non-nanosecond casting behavior for `expand_dims` (pydata#8782)
  Migrate treenode module. (pydata#8757)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Mar 16, 2024
* main: (42 commits)
  correctly encode/decode _FillValues/missing_values/dtypes for packed data (pydata#8713)
  Expand use of `.oindex` and `.vindex` (pydata#8790)
  Return a dataclass from Grouper.factorize (pydata#8777)
  [skip-ci] Fix upstream-dev env (pydata#8839)
  Add dask-expr for windows envs (pydata#8837)
  [skip-ci] Add dask-expr dependency to doc.yml (pydata#8835)
  Add `dask-expr` to environment-3.12.yml (pydata#8827)
  Make list_chunkmanagers more resilient to broken entrypoints (pydata#8736)
  Do not attempt to broadcast when global option ``arithmetic_broadcast=False`` (pydata#8784)
  try to get the `upstream-dev` CI to complete again (pydata#8823)
  Bump the actions group with 1 update (pydata#8818)
  Update documentation for clarity (pydata#8817)
  DOC: link to zarr.convenience.consolidate_metadata (pydata#8816)
  Refactor Grouper objects (pydata#8776)
  Grouper object design doc (pydata#8510)
  Bump the actions group with 2 updates (pydata#8804)
  tokenize() should ignore difference between None and {} attrs (pydata#8797)
  fix: remove Coordinate from __all__ in xarray/__init__.py (pydata#8791)
  Fix non-nanosecond casting behavior for `expand_dims` (pydata#8782)
  Migrate treenode module. (pydata#8757)
  ...
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