ofind finds non-unique cell magics #1906
Conversation
| @@ -50,6 +50,7 @@ | |||
| ESC_SH_CAP = '!!' | |||
| ESC_HELP = '?' | |||
| ESC_MAGIC = '%' | |||
| ESC_CELL_MAGIC = ESC_MAGIC * 2 | |||
There was a problem hiding this comment.
This might be a good time to fix the fact that all these escapes are defined twice, once here and once in inputsplitter. Let's remove the duplication and have these things in a single place...
There was a problem hiding this comment.
Sure. Which one makes more sense? (this is part of the code I have never spent much time in).
There was a problem hiding this comment.
I had also thought of just calling it ESC_MAGIC2 to keep it shorter and as a mnemonic that we're really making it be always the first twice. Up to you...
There was a problem hiding this comment.
Is that defined? I wasn't sure if it was strictly defined as MAGIC*2, or MAGIC+foo, or what. I just remembered a recent conversation where there was a question about override, so I made the code assume as little as possible, because I am least familiar with this part of the code. But ESC_MAGIC2 seems fine to me.
There was a problem hiding this comment.
I think inputsplitter, if nothing else because that code is a bit more cleanly organized and designed, where as prefilter is more of an attempt to clean up old hacks from the very early days of IPython (and it does partly succeed, but it's still pretty messy).
There was a problem hiding this comment.
These things are constants defined here, but in practice they're pretty much hardcoded: the amount of hand-coded logic and regexps everywhere that assumes these values is so big that we might as well call them 'the ipython language' once and for all and drop any pretense that they will ever be user-tunable.
There was a problem hiding this comment.
Are they solid enough that the clearly redundant test of if not startswith(ESC_CELL...) and not startswith(ESC_MAGIC) should be removed?
There was a problem hiding this comment.
Yup. That stuff can be changed by whoever forks ipython into a new project and redoes it from the inside out :)
|
Might be a good idea to make a test that defines a pair of line/cell magics implemented like you did px (two functions instead of one) and see that we recover their docstrings correctly. We're likely to still fiddle with the magics machinery, and I'd feel better knowing we have some more coverage of the internals (esp. on something like this we already missed once). |
duplicate defs in prefilter are now imported from inputsplitter
|
okay, ESC_FOO now defined in inputsplitter, and test added for line/cell homonyms. |
|
Great job, ran tests, all OK. Rest of code looks solid, thanks for adding the tests. I feel much better about that. Merging now. |
Fixes logic in `Shell._ofind` to differentiate cell magics from line magics. Also changes preemptive not-found to not fire in case of magics, as it has been pointed out elsewhere that magic names need only not contain spaces, they need not be valid identifiers. Closes #1904.
Fixes logic in `Shell._ofind` to differentiate cell magics from line magics. Also changes preemptive not-found to not fire in case of magics, as it has been pointed out elsewhere that magic names need only not contain spaces, they need not be valid identifiers. Closes ipython#1904.
Fixes logic in
Shell._ofindto differentiate cell magics from line magicsdefines ESC_CELL_MAGIC in prefilter, rather than making the assumption that ESC_CELL_MAGIC == ESC_MAGIC * 2, but I don't have to do that.
also changes preemptive not-found to not fire in case of magics, as it has been pointed out elsewhere that magic names need only not contain spaces, they need not be valid identifiers.
closes #1904