include accessors in __dir__
#9985
Conversation
|
(the failing Otherwise this is ready for reviewing |
xarray/core/common.py
Outdated
| @@ -351,7 +351,7 @@ def __dir__(self) -> list[str]: | |||
| for item in source | |||
| if isinstance(item, str) | |||
| } | |||
| return sorted(set(dir(type(self))) | extra_attrs) | |||
| return sorted(set(dir(type(self))) | type(self)._accessors | extra_attrs) | |||
There was a problem hiding this comment.
this has become a bit messy: while _accessors is a class variable, it is only available on its subclass. To really make sense we'd have to override DataTree.__dir__, DataArray.__dir___ and Dataset.__dir__ with
def __dir__(self):
return sorted(set(super().__dir__()) | type(self)._accessors)or create a new mixin containing _accessors and have all three classes inherit from that (after AttrAccessMixin, so super().__dir__() dispatches to that).
However, set(dir(type(self))) already uses a similar trick (defer to the subclass type's __dir__) so we may also choose to ignore the typing.
There was a problem hiding this comment.
What stops you from adding _accessors: ClassVar[set[str]] to this mixing class?
There was a problem hiding this comment.
mostly that this is a mixin specialized on providing attribute-based syntax (using .) for entries in attrs. I guess it would be best to have another mixin for the accessors
There was a problem hiding this comment.
I've created a separate mixin, which allows a separation of concerns (and makes removing the AttrAccessMixin easier, should we ever wish to). The downside is that now we have one more base class / mixin.
xarray/core/common.py
Outdated
| @@ -351,7 +351,7 @@ def __dir__(self) -> list[str]: | |||
| for item in source | |||
| if isinstance(item, str) | |||
| } | |||
| return sorted(set(dir(type(self))) | extra_attrs) | |||
| return sorted(set(dir(type(self))) | type(self)._accessors | extra_attrs) | |||
There was a problem hiding this comment.
What stops you from adding _accessors: ClassVar[set[str]] to this mixing class?
| @@ -709,6 +709,7 @@ class Dataset( | |||
| _close: Callable[[], None] | None | |||
| _indexes: dict[Hashable, Index] | |||
| _variables: dict[Hashable, Variable] | |||
| _accessors: ClassVar[set[str]] = set() | |||
There was a problem hiding this comment.
I think using a mutable default is bad practice.
But the only issue I can imagine is custom subclasses that mess with this? So maybe it's fine.
There was a problem hiding this comment.
yeah, the only issue would be that subclasses inherit this and try to modify this. However, if they do it would probably be better to just have them override the class variable.
(Additionally, as far as I understand the class is an instance of the metaclass, so having the usual "sentinel then switch to mutable on first access" doesn't make sense for class variables)
| assert "demo" in dir(xr.DataTree) | ||
| assert "demo" in dir(xr.Dataset) | ||
| assert "demo" in dir(xr.DataArray) |
There was a problem hiding this comment.
Does this PR also allow completion of the actual accessor methods, or just the accessor namespace? If so then we should test that too.
There was a problem hiding this comment.
it does, but but how would you test that? This is functionality of the auto-completer...
Edit: to be clear, I think auto-completion of the namespace was already working before this PR
There was a problem hiding this comment.
actually, I just tried dir(ds) on main, and it included the accessor... so it might be that this PR is not necessary? Can anyone confirm that? Or did I manage to confuse myself?
There was a problem hiding this comment.
it does, but but how would you test that?
Surely just
ds = xr.Dataset()
assert "foo" in dir(ds.demo)?
I just tried dir(ds) on main, and it included the accessor... so it might be that this PR is not necessary? Can anyone confirm that? Or did I manage to confuse myself?
I just tried seeing if xr.Dataset().d would suggest xr.Dataset().dt (because that's a built-in accessor) and for me it does not.
There was a problem hiding this comment.
dt and str are only on DataArray
Edit: And assert "foo" in dir(ds.demo) should be the same as assert "foo" in dir(DemoAccessor), no?
There was a problem hiding this comment.
if can, also try with virtualizarr or another third-party accessor, the built-in accessors are a bit different from the ones registered by register_*_accessor
There was a problem hiding this comment.
Oh right, yeah that works for me (in an ipython terminal). @chuckwondo you said you saw different behaviour? If so what is your dev setup?
Yes, in the REPL, auto-complete works. I can hit tab after "ds." and have "virtualize" show up, which means that even without the PR, "virtualize" is already in "dir."
However, this is not the case within the IDE (I'm using VS Code).
I suspect that's because in the REPL, the import of virtualizarr is actually executed, thus the accessor is registered, but within the IDE, no import actually takes place in the context of what is made available for code-completion.
There was a problem hiding this comment.
Although, I think I'm now also a bit confused, as I would think that within the IDE there's still something that has to be evaluated with regard to decorators in order for the final definition of the things that are decorated to be available to the IDE.
There was a problem hiding this comment.
if I understand correctly, VSCode builds its code completion on type stubs and type inference (through pylance), which means that dynamic attributes like accessors may not be possible to autocomplete.
@max-sixty, if I remember correctly, you're also using VSCode? Do you have any additional insight here?
There was a problem hiding this comment.
Are any of you using a different IDE that does show the expected code-completion suggestion?
We were not including accessors in
__dir__so far, which would mean that dynamic code completion was not suggesting any accessors.This can't make use of
_cache, which is related but an instance variable, and will only be populated on the first access of each accessor (by_CachedAccessor).whats-new.rst