Fix paste/cpaste bug and refactor/cleanup that code a lot. #1007

Merged
merged 3 commits into from Nov 26, 2011

Projects

None yet

3 participants

@fperez
IPython member

In fixing a pasting bug (mishandling of whitespace when input had
prompts) it became clear the pasting code hadn't been updated
when the new prefiltering machinery was added. Furthermore, the
pasting magics are only for the terminal, but the code was in the
base classes.

This refactors and simplifies the pasting code, moving it to the
terminal shell only, and removing unnecessary methods from the
main class (using small utility functions instead).

The tests were simplified because the previous regexp supported some
odd edge cases that are not valid in the normal prefiltering code. We
want to have a single location and set of rules for input
prefiltering, so I changed some of the test cases to be consistent
with what prefilter allows.

@fperez
IPython member

@stefanv, could you have a quick look and see if this fixes correctly the cases you'd seen?

@minrk
IPython member

This is looking pretty good to me. One general question - store_or_execute() and rerun_pasted() are functions whose first argument is always the object that calls them, making them perfectly identical to instance methods in terms of implementation. What benefit does this approach have over them just being methods?

In this way, every single method of an object could be made a function. For future reference, which ones should be functions, and which should be methods?

@fperez
IPython member

They were actually methods, and you are totally correct that by this token we could completely strip the object down :) I don't have a hard rule yet and I'm just feeling my way through this, but it seems to me that our main interactive shell objects have become gigantic and unwieldy, so I was trying to reduce their method footprint and API a little bit.

Since these were little pieces of functionality only used by the terminal application, it seemed like a cleaner solution to just refactor them out into separate standalone functions. That would also make it much easier to test them in isolation with mock objects that only provide one or two methods, something harder to do when they are methods of the main thing.

I'm not trying to go and rip out everything from the main shell object, but these didn't really feel like 'core' functionality (in fact they were _private in the previous implementation). So it's really mostly an instinct design call, and one that I'd love feedback on. Do you agree in general with trying to (when reasonably easy and non-disruptive as was the case here) simplify a little bit the official public API of our big Shell object, and with this approach? I mostly want us to find a good design balance for this that will make the overall use and maintenance of the code easier...

@stefanv

E-mail pasting no longer works, e.g.

>> def foo(x):
>>     return x

Also, some valid Python causes confusion:

>>> def foo(x): return 1 \
...     > 1
@minrk
IPython member

@fperez - I agree that we should move in a direction of more functions and fewer methods, but these seem like they really are methods, since copying/pasting them into the Class scope would require no internal changes at all (aside from changing invocation back from f(self to self.f(). They are still highly stateful, depending on and affecting the active state of the shell instance and various members. I'm not sure that anything that calls shell.run_cell() is better off being split off into a function.

I don't feel strongly and I don't know where the line should be drawn, but my first inclination is that if you have to provide the shell instance itself, rather than some of its members, it's probably a method.

@fperez
IPython member

Just rebased to sort out a conflict and force-pushed. Too tired now to work any more on this, will get back to it in a couple of days.

fperez added some commits Nov 17, 2011
@fperez fperez Fix paste/cpaste bug and refactor/cleanup that code a lot.
In fixing a pasting bug (mishandling of whitespace when input had
prompts) it became clear the pasting code hadn't been updated
when the new prefiltering machinery was added.  Furthermore, the
pasting magics are only for the terminal, but the code was in the
base classes.

This refactors and simplifies the pasting code, moving it to the
terminal shell only, and removing unnecessary methods from the
main class (using small utility functions instead).

The tests were simplified because the previous regexp supported some
odd edge cases that are not valid in the normal prefiltering code.  We
want to have a single location and set of rules for input
prefiltering, so I changed some of the test cases to be consistent
with what prefilter allows.
8bb887c
@fperez fperez Move tests for magics that are terminal-specific to their own file. 8259953
@fperez fperez Add tests for email quote stripping.
Also, clarify why certain utilities are kept as standalone functions
instead of making them methods.
84830ec
@fperez
IPython member

@stefanv, I fixed the email case and added some tests for that kind of format, let me know if this looks OK now.

@minrk, here's how I view the ones in this particular case: while I agree with you that those are basically methods in all but name right now, I hope that soon we'll be able to refactor our magic system into standalone objects. And in this case, those methods are really utilities of the paste/cpaste magics, not of the shell itself. If those magics were in a separate object (say one added by a user in an extension), they would never modify the shell as methods, they'd simply pass it as an argument. So I think that leaving them as standalone functions now is the right thing to do, in anticipation for an eventual refactoring into a new object, a Magic, that would put them together with paste/cpaste.

If you guys agree with this, I'll go ahead and merge. Thanks for the review!

@minrk
IPython member

@fperez - I think this is fine. It does seem a bit odd to pull methods out into functions, with the ultimate goal that they become methods of a different object in the future. I do appreciate the desire to pull things off of the Shell, so it makes sense.

If @stefanv's says his cases work, go for it.

@fperez
IPython member

Thanks, @minrk. I did add a comment around them to try and clarify the intent of the somewhat odd refactoring. I hope to find the time soon to draft my ideas on the magics and post them on the dev list for feedback.

@stefanv, note that I fixed email pasting, but not that very odd corner case you found. The logic for handling continuation lines gets pretty complicated if we want to mix in automatic removal of special characters, so I think it's OK if the pasting utilities don't cover them. I don't want them to become overly complex. As long as we handle that code in the normal input prompt, I think that's an acceptable compromise. What do you think?

@stefanv

@fperez Thanks, the e-mail case is the most common one I deal with. Dealing with all the corner cases cleanly may be very hard, so I'm be happy with the current behaviour.

@fperez
IPython member

Great, merging and closing. Thanks for the feedback!

@fperez fperez merged commit 780b7c5 into ipython:master Nov 26, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment