Skip to content
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

Remove runlines etc. #373

Merged
merged 7 commits into from Apr 13, 2011
Merged

Remove runlines etc. #373

merged 7 commits into from Apr 13, 2011

Conversation

takluyver
Copy link
Member

This removes a few old methods, and tidies up the last few calls to ip.runlines().

I think we can also remove the reset_buffer method, and the InteractiveShell's buffer and buffer_raw attributes: grepping through the code, it looks like nothing that remains ever puts anything into those buffers. But there's still a few bits that check the buffers (e.g. in prefilter), and I just wanted to check that those bits are safe to remove.

@ellisonbg
Copy link
Member

Can we also remove InteractiveShell.autoindent? It has a comment in
the code saying that it is used on runlines, but I am not sure we are
totally free of it. If it is easy to remove, we should do so at this
point.

On Tue, Apr 12, 2011 at 2:55 PM, takluyver
reply@reply.github.com
wrote:

This removes a few old methods, and tidies up the last few calls to ip.runlines().

I think we can also remove the reset_buffer method, and the InteractiveShell's buffer and buffer_raw attributes: grepping through the code, it looks like nothing that remains ever puts anything into those buffers. But there's still a few bits that check the buffers (e.g. in prefilter), and I just wanted to check that those bits are safe to remove.

Reply to this email directly or view it on GitHub:
#373

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@takluyver
Copy link
Member Author

It still seems to be tied to our actual autoindenting, so I don't think so,
unfortunately.

@ellisonbg
Copy link
Member

OK, thanks for checking

On Tue, Apr 12, 2011 at 3:13 PM, takluyver
reply@reply.github.com
wrote:

It still seems to be tied to our actual autoindenting, so I don't think so,
unfortunately.

Reply to this email directly or view it on GitHub:
#373 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@fperez
Copy link
Member

fperez commented Apr 12, 2011

go for buffer removal, if it's safe.

Basically, when I wrote inputsplitter and built the new machinery, I was a bit afraid of tearing down everything at the same time: if something went badly wrong with the new system, I wanted to have an easy way to back off (without full version control reverts), so I left all the dead code around. And just hadn't gotten around to removing it...

So aggressive removal of anything there that seems unused is ok. However, in addition to running the test suite, also do a little bit of old-style manual interactive testing just for extra safety: type a few lines, indented stuff, magics, etc... Write out a little file you can paste from to make life easier if you want...

But it will be great to get rid of this old code, so we can actually see our new shiny building :) Thanks!

@takluyver
Copy link
Member Author

Good call, manual testing found one little thing I'd been too hasty to change (in the terminal frontend, now resolved). As far as I can tell, this all behaves properly now, but please do hunt out any corner cases.

@takluyver
Copy link
Member Author

(Once again tagging small changes onto an unrelated pull request - I shall try to stop needing to do this)

@fperez
Copy link
Member

fperez commented Apr 12, 2011

Any chance you could add tests for the corner cases you found, or were they the kind of thing that only actual manual, interactive typing would show?

@takluyver
Copy link
Member Author

I don't think it can be automated. Specifically, if you start a multiline cell (e.g. for a in range(5): ), then press Ctrl-C, the existing contents should be discarded. I tried to remove this line, and that broke: #373 (comment)

@fperez
Copy link
Member

fperez commented Apr 12, 2011

Ctrl-C can be emulated with a 'raise KeyboardInterrupt', would that help?

@takluyver
Copy link
Member Author

I don't think so, it has to be triggered in the user input part of the
terminal REPL.

@fperez
Copy link
Member

fperez commented Apr 12, 2011

On Tue, Apr 12, 2011 at 4:55 PM, takluyver
reply@reply.github.com
wrote:

I don't think so, it has to be triggered in the user input part of the
terminal REPL.

ah, ok. let me give it a quick test as well...

@fperez
Copy link
Member

fperez commented Apr 13, 2011

sorry for the delay, had some network problems at the office.

Looks good here, other than a failure in the parallel tests, but that's in master too, so no problem on this side.

Thanks!

@takluyver takluyver merged commit 72be7c7 into ipython:master Apr 13, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants