-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add new interactivity mode for ast. #10598
Conversation
@rgbkrk though on that ? I though about that because display() is long to type and I often do
Just to display it. |
cc82cfc
to
2e8d986
Compare
Up until 6.0 the following code wouldn't show the value of the assigned | ||
variable: | ||
|
||
>>> var = "something" |
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.
My brain almost broke because it was in JS mode while reading this.
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 can change for let
if you want ?
If this behavior exists, out of old Matlab habit I will append I too do the operation then display it pretty often. I'd hate to have to figure out how to suppress output though. |
Oh I see, this isn't the default setting, just a configurable. 🆒 |
yeah, of course it's not the default :-)
|
IPython/core/interactiveshell.py
Outdated
if isinstance(nodelist[-1], ast.Assign): | ||
asg = nodelist[-1] | ||
if len(asg.targets) == 1: | ||
nnode = ast.Expr(ast.Name(asg.targets[0].id, ast.Load())) |
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 this will crash if you try to do a multiple assignment. When I parse something like (a, b) = (1, 2)
, I get an assignment with a length-1 list of targets whose only entry is an instance of ast.Tuple
, which has no id
:
In [5]: sys.version_info
Out[5]: sys.version_info(major=3, minor=5, micro=2, releaselevel='final', serial=0)
In [6]: %%ast
...: (a, b) = (1, 2)
...:
Module(
body=[
Assign(
targets=[
Tuple(
elts=[
Name(id='a', ctx=Store()),
Name(id='b', ctx=Store()),
],
ctx=Store(),
),
],
value=Tuple(
elts=[
Num(1),
Num(2),
],
ctx=Load(),
),
),
],
)
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.
(That %%ast
magic comes from https://github.com/llllllllll/codetransformer/blob/master/codetransformer/__init__.py#L23-L35, btw. That might be relevant to your interests if you're working on ast stuff.)
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.
Oh, sweet. Thanks. Does it also define a pretty formatter for ast objects that are returned ?
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 doesn't at the moment, but that could be added pretty easily. The logic that does the actual formatting lives here: https://github.com/llllllllll/codetransformer/blob/master/codetransformer/utils/pretty.py#L31.
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.
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.
Oh, TIL. @llllllllll we should probably do a codetransformer release...
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.
1f5d398
to
5260f5a
Compare
Thanks ! Though I'm on 3.6, so can't import it :-( Pushed new commits that test the various behavior. |
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.
Nice work 👍
With the option ``InteractiveShell.ast_node_interactivity='last_expr_or_assign'`` | ||
you can now do:: | ||
|
||
>>> df = pandas.load_csv(...) |
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.
Make this example match the others above? And use IPython prompts instead of >>>
?
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.
done.
IPython/core/interactiveshell.py
Outdated
@@ -2781,6 +2787,15 @@ def run_ast_nodes(self, nodelist, cell_name, interactivity='last_expr', | |||
if not nodelist: | |||
return | |||
|
|||
if interactivity == 'last_expr_or_assign': | |||
if isinstance(nodelist[-1], ast.Assign): |
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 this is fine for getting this feature in and letting people experiment with it, but be aware that it won't catch a += 1
(which is an AugAssign
node), or a : int = 1
(new in Python 3.6, AnnAssign
node).
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.
Good call. Fixed. It should work on all of these.
435916a
to
faaa345
Compare
ip.run_cell('(u,v) = (41+1, 43-1)') | ||
|
||
finally: | ||
ip.run_cell('a = 1+1', store_history=True) |
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.
Why is this in the finally block?
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.
No reasons, bad copy past likely. Fixed.
faaa345
to
81a07e3
Compare
@fperez would prefer for this to not assign to |
Hum inject something at the end like that:
At the same time when the expression of a of a cell display something there is always an |
If we're injecting Maybe there's a nicer way to do this - instead of modifying the AST, inspect it to find the target of the final assignment, and then record some information which says 'if this code completes successfully, print the value of |
Well that's almost exactly what the above do. it calls display on the last assignment target. Here is my argument against having it displayed and not triggerd as part of display hook. One the the two below methods are doing def load_something():
display(...) The other def load_somethingelse():
return ... $ ipython
Python 3.6.0 |Continuum Analytics, Inc.| (default, Dec 23 2016, 13:19:00)
Type 'copyright', 'credits' or 'license' for more information
IPython 6.1.0.dev -- An enhanced Interactive Python. Type '?' for help.
In [1]: %config TerminalInteractiveShell.ast_node_interactivity='display_last_expr_or_assign'
In [2]: from dummymod import load_cars, load_iris
In [3]: a = load_iris()
=================
Id | types
=================
1 | A
2 | B
3 | A
None
In [4]: b = load_cars()
=================
Id | types
=================
I | E
II | F
III | G With the current implementation that would be disambiguated by having (or not) an Out: In [6]: a = load_iris()
=================
Id | types
=================
1 | A
2 | B
3 | A
In [7]: b = load_cars()
Out[7]:
=================
Id | types
=================
I | E
II | F
III | G Well now both are implemented so I can push 2 options. |
Ok I'm totally going to set this in my personal defaults. |
Note, the current code will also not work if the LHS is an arbitrary assignment like |
So |
ca387c8
to
81a07e3
Compare
you can now do:: | ||
|
||
In[2]: xyz = "something else" | ||
"something else" |
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.
This example (and the second one above) should have an Out[2]:
prompt, right?
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.
Yes ! I s/>>>/In[1]
stupidly.
81a07e3
to
2a668e5
Compare
Allow to display the result of the last assignment of it has a unique target.
2a668e5
to
a137755
Compare
Now that 6.1 is released, I'm getting this in to play with it. |
Allow to display the result of the last assignment of it has a unique
target.
meh, I should write tests for that.