Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
include accessors in
__dir__
#9985base: main
Are you sure you want to change the base?
include accessors in
__dir__
#9985Changes from all commits
5314f02
4dc0527
bc5a25b
9433b64
2b7d0c8
050d834
9862105
580fd3f
bdc7984
92d9f89
336eb26
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, I just tried
dir(ds)
onmain
, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely just
?
I just tried seeing if
xr.Dataset().d
would suggestxr.Dataset().dt
(because that's a built-in accessor) and for me it does not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dt
andstr
are only onDataArray
Edit: And
assert "foo" in dir(ds.demo)
should be the same asassert "foo" in dir(DemoAccessor)
, no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are any of you using a different IDE that does show the expected code-completion suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @keewis is right, most IDEs nowadays support auto-completion via LSP servers based on (somewhat strict) static code analysers such as pyright / pylance. So I'm afraid there's nothing much we can do to support auto-completion for dynamic attributes.
IDE configs using jedi under the hood may support it, although IIUC jedi operates under different modes (Interpreter vs. Script) so I'm not sure auto-completion for dynamic attributes will always work (probably only in the Interpreter mode?).
Decorators are just syntax sugar for functions applied on other functions or classes, so if the decorator function is properly typed (i.e., its return type is clear or can be statically inferred) any static code analyser will be able to provide auto-completions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to enable this for mypy here: #7117
But failed because the documentation on mypy plugins is non existent.
Maybe something similar is possible for pyright, pylance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the insight @benbovy. Sadly, for me at least, this seems to confirm one of the reasons that I'm not a fan of dynamically registered accessors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, just saw this.
This is also my understanding — a kernel will give better info than an IDE because it's doing something like calling
dir(foo)
rather than statically analyzing the code...