New input transformation machinery#11041
Conversation
This can obscure underlying errors
This should probably have some tests.
This allows for fewer pieces to know about input_transformer_manager.
willingc
left a comment
There was a problem hiding this comment.
@takluyver This looks really good. The code looks clean. Most of my suggestions are related to docstrings and improving understanding. After merging we should probably go back and do another pass with docs. Thank you 👍
| """Input transformer machinery to support IPython special syntax. | ||
|
|
||
| This includes the machinery to recognise and transform ``%magic`` commands, | ||
| ``!system`` commands, ``help?`` querying, prompt stripping, and so forth. |
There was a problem hiding this comment.
Perhaps add to comment:
Added: IPython 7.0. Replaces inputsplitter and inputtransformer which were deprecated in 7.0.
IPython/core/inputtransformer2.py
Outdated
| """Remove leading indentation. | ||
|
|
||
| If the first line starts with a spaces or tabs, the same whitespace will be | ||
| removed from each following line. |
IPython/core/inputtransformer2.py
Outdated
| Parameters | ||
| ---------- | ||
| prompt_re : regular expression | ||
| A regular expression matching any input prompt (including continuation) |
IPython/core/inputtransformer2.py
Outdated
| initial_re : regular expression, optional | ||
| A regular expression matching only the initial prompt, but not continuation. | ||
| If no initial expression is given, prompt_re will be used everywhere. | ||
| Used mainly for plain Python prompts, where the continuation prompt |
There was a problem hiding this comment.
plain Python prompts (typically >>>), where the
IPython/core/inputtransformer2.py
Outdated
| return ['get_ipython().run_cell_magic(%r, %r, %r)\n' | ||
| % (magic_name, first_line, body)] | ||
|
|
||
| # ----- |
IPython/core/interactiveshell.py
Outdated
|
|
||
| @property | ||
| def input_splitter(self): | ||
| """Make this available for compatibility |
There was a problem hiding this comment.
Make this available for backward compatibility (pre-7.0 release) with existing code.
For example, ipykernel...
New code should use input_transformer_manager.
There was a problem hiding this comment.
@willingc Your 7 and 0 keys are either swapped, or you're used to software below 1.0 :-) I've edited a couple of your comments to say 7.0 instead of 0.7 as I think that's what you meant.
There was a problem hiding this comment.
Yes, I had a backwards moment 😉
| ------- | ||
| result : :class:`ExecutionResult` | ||
| """ | ||
| result = None |
|
|
||
| return result | ||
|
|
||
| def transform_cell(self, raw_cell): |
There was a problem hiding this comment.
Add docstring explaining static input transformation vs dynamic transformations
| @@ -0,0 +1,189 @@ | |||
| import nose.tools as nt | |||
There was a problem hiding this comment.
Add a docstring similar to this:
"""Tests for the token-based transformers in IPython.core.inputtransformer2
Line-based transformers are the simpler ones; token-based transformers are
more complex.
"""
IPython/terminal/shortcuts.py
Outdated
|
|
||
| kb.add('c-o', filter=(has_focus(DEFAULT_BUFFER) | ||
| & emacs_insert_mode))(newline_autoindent_outer(shell.input_splitter)) | ||
| & emacs_insert_mode))(newline_autoindent_outer(shell.input_transformer_manager)) |
There was a problem hiding this comment.
A comment on style here. Perhaps going to a longer line length would improve readability and understanding. i.e. line 74 is more understandable than 71-72 and 66-70
|
I'll re-read all of that later, flying tomorrow. If I have wifi in the plane, I'll do that then. |
|
I push a docstring and a couple of deprecation warnings. |
|
Thanks @willingc for the detailed review! I think I've basically applied all of your suggestions. |
|
cc @dakoop, @colinjbrown from https://github.com/dataflownb/dfkernel I believe you are using input transformer. Just to make sure this will not affect you too much. |
|
@Carreau, thanks for the heads-up. Shouldn't affect us much, and it looks like a nice improvement. |
|
@takluyver Traveling today but trust that you covered all of my suggestions. I'll give a look tomorrow. |
|
@takluyver, I'd like to try to cut a 7.0 current september. We had no complaint so I'm assuming everyone is +1, so I suggest fixing conflict (relatively trivial), merging and moving on. With that we can get a beta for about a week, then RC and final by hopefully mid to late september. How does that sound ? |
|
I've fixed the conflicts. |
I've fixed new conflict due to the reactivation of autoindent. |
|
Thanks for fixing up the conflicts; I would be happy to see this land (though I'm not looking forward to finding out what the change has broken). |
So be it,
I'll help you fix stuff, worse case we revert and re-iterate. There is only a couple of things left for 7.0 (and one test regression I'd like to backport #11277 for 5.9) and we can do a beta. |
|
🎉 🍸 :pinot grigio: |
|
I'm sure even Mr Meeseeks is excited... |
|
congratulation Thomas, good work ! |
|
|
|
:-P Does saying the bots name and the word 'excited' in the same comment trigger that? |
|
No, but the bot can read inside HTML comments, that aren't shown in the UI!
…On Sun, Sep 2, 2018, 00:27 Thomas Kluyver ***@***.***> wrote:
:-P
Does saying the bots name and the word 'excited' in the same comment
trigger that?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#11041 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAUez322zIhNiwJpySGHfGI1iHKc5YFoks5uW4hjgaJpZM4SlpR3>
.
|
See #10272. This is a major change which I expect to be the trigger for IPython 7.0.
Main changes
IPython.core.inputtransformer2is added, containing the new machinery to transform input before we parse it.IPython.core.inputsplitterandIPython.core.inputtransformerare deprecated. We can't remove them immediately because nbconvert relies on them.IPython.utils.tokenize2is removed. This was a copy of the stdlibtokenizemodule with a couple of issues patched. One of them (bpo 12691) was fixed in Python 3.4.1, which we will require for IPython 7 (see Drop support for Python 3.3 #10833). The other (bpo 17061) turned out to be intended behaviour, so I've handled it another way in inputtransformer2.Rationale
This will break any code that extends the current input transformation machinery (and the interface was documented), and any change this big is bound to cause some unexpected bugs. So why should we do this?
Limitations
%line_magics,!system commands,help?, etc.) results in another tokenisation pass after the transformation. So a large code cell with many instances of special syntax could take longer to parse than it currently does - although the current code isn't very efficient either (see above). I haven't tried to measure this. My intuition is that large cells tend to be regular Python code, as in infinite loop #11026, in which case the new implementation can comfortably beat the old one.