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
Support tab completion in Jupyter by inserting aliases into the method signature #1282
Support tab completion in Jupyter by inserting aliases into the method signature #1282
Changes from 6 commits
8d21c1b
8082150
49aa7b5
c2a2979
c6477d7
d3a046e
cd12847
dc25707
f37faea
377ba97
6ed7e93
3c63ecc
d0acb50
fcc0e34
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.
Perhaps we should move the code into the
use_alias
decorator, because we only need to "insert aliases into the signature" when aliases are defined by@use_alias
? It also means all functions support tab completion without any further actions (i.e.,adding @insert_alias
).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 agree that combinging
use_alias
andinsert_alias
is the smart move, since I don't see howinsert_alias
would be used on its own.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 like the idea of implementing this inside
use_alias
to simplify the diff, but would prefer to not combine by simply moving the code insideuse_alias
. As an alternate solution, my latest commit callsinsert_alias
from withinuse_alias
. There are two reasons why I prefer this option to having all the code withinuse_alias
. First, I think that it is simpler (i.e., easier to maintain) if individual functions do smaller, isolated tasks. Second, this implementation is compatible with the changes proposed in #1282. Even if that PR doesn't get merged, it seems to me that havinginsert_alias
as a separate function could be more compatible with future updates to the decorators (reason 1).