PR: Run cells through a function instead of pasting their contents to the console #7310

bcolsen · 2018-06-19T07:08:05Z

Pull Request Checklist

Read and followed this repo's Contributing Guidelines
Based your PR on the latest version of the correct branch (master or 3.x)
Followed PEP8 for code style
Ensured your pull request hasn't eliminated unrelated blank lines/spaces,
modified the spyder/defaults directory, or added new icons/assets
Wrote at least one-line docstrings for any new functions
Added at least one unit test covering the changes, if at all possible
Described your changes and the motivation for them below
Noted what issue(s) this pull request resolves, creating one if needed
Included a screenshot, if this PR makes any visible changes to the UI

Description of Changes

This PR makes a function that runs a cell instead of copying the cell contents into the current console. With this function the tracebacks give the correct lines in the file the cell was run from!! Also if the file is run in a dedicated console the code cells will also run in that console regardless of the console in focus.

Implementation

A variable with the cell contents is made in the ipython namespace of the console so there will be no interference with user variables. The function executes this code then deletes the variable. The function displays an error if it is run from the console. I can leave the variable so the function can run the last cell, but it's not obvious that it won't reflect any subsequent changes to the block in the editor.

Issue(s) Resolved

Fixes #7113
Fixes #7760

pep8speaks · 2018-06-19T07:08:14Z

Hello @bcolsen! Thanks for updating the PR.

In the file spyder/app/tests/script.py, following are the PEP8 issues :

Line 12:1: E265 block comment should start with '# '

In the file spyder/app/tests/test_mainwindow.py, following are the PEP8 issues :

Line 869:9: E128 continuation line under-indented for visual indent

There are no PEP8 issues in the file spyder/config/main.py !
There are no PEP8 issues in the file spyder/plugins/editor/plugin.py !
There are no PEP8 issues in the file spyder/plugins/editor/widgets/base.py !
There are no PEP8 issues in the file spyder/plugins/editor/widgets/editor.py !
In the file spyder/plugins/ipythonconsole/plugin.py, following are the PEP8 issues :

Line 937:29: E127 continuation line over-indented for visual indent
Line 938:33: E128 continuation line under-indented for visual indent
Line 938:72: E231 missing whitespace after ','
Line 964:33: E131 continuation line unaligned for hanging indent
Line 965:33: E128 continuation line under-indented for visual indent

There are no PEP8 issues in the file spyder/plugins/outlineexplorer/widgets.py !
There are no PEP8 issues in the file spyder/utils/syntaxhighlighters.py !

Comment last updated on October 27, 2018 at 20:09 Hours UTC

ccordoba12 · 2018-06-23T23:40:32Z

@bcolsen, thanks for this, it looks very interesting! But since we're moving all our kernel code to this new package:

https://github.com/spyder-ide/spyder-kernels

(see also PR #7306), you need to create a PR there for your changes to our sitecustomize. Please do it against the master branch of spyder-kernels because the 0.x branch is meant for Spyder 3.

bcolsen · 2018-06-25T07:03:05Z

@ccordoba12 I rebased and made a PR against the spyder-kernels repo and everything still works.

There is still some code repetition in the functions and I need to add/fix the tests, but first I would like to know if my approach here is viable before cleaning it up.

It works well for my use cases and I'm going to try to use it at work this next week to test it out

ccordoba12 · 2018-08-24T18:45:46Z

@bcolsen, I'm very sorry for not reviewing your work for so long. I took a quick look at the current status of this PR at it looks quite good (minus some refactoring to avoid repeated code, as you said).

Please merge with master to fix the conflicts and implement a test for this.

ccordoba12 · 2018-08-24T18:47:34Z

Also, it should be possible to turn on/off the old behavior with an option.

ccordoba12 · 2018-08-29T10:48:32Z

Ok, I assume you need the corresponding PR in spyder-kernels to be merged first for the tests here to pass, right?

I'l, try to take a look at that as soon as I can.

bcolsen · 2018-08-29T14:39:36Z

Yes I'll need kernel side merged to pass the tests, but it not quite ready yet.

I'll focus on that code next. I'll let you know when it's ready for review.

CAM-Gerlach · 2018-09-04T06:12:09Z

@bcolsen Since Spyder-Kernels was merged a while back, are you planning to finish this one? Any updates? Thanks!

bcolsen · 2018-09-04T06:24:38Z

The the corresponding spyder-kernel PR is spyder-ide/spyder-kernels#7 has not yet been merged, but is waiting for review. That's why it's failing tests.

I have one more fuction to clean up in the PR and tests, then it's good to go. I think I can finish it up this week.

bcolsen · 2018-09-05T05:01:02Z

One other note on this implementation that could be seen as a regression.

Unlike the current method of running cells this method is unable to have the last line of the cell produce output in the IPython console.

Output is only produced from print functions and other methods of writing to stdout and stderr just like running a file.

bcolsen · 2018-09-08T05:18:04Z

spyder/plugins/ipythonconsole/plugin.py

@@ -1025,6 +1026,61 @@ def run_script(self, filename, wdir, args, debug, post_mortem,
                  "<br><br>Please open a new one and try again."
                  ) % osp.basename(filename), QMessageBox.Ok)

+    def run_cell(self, code, cell_name, filename):


This function has some similar code to run_script above but I would need to re-factor both functions to not have some repeat. I'm not should what all the code in run script does so I'm hesitant to touch it too much.

Ok, I'll try to refactor both methods here in your branch, after you address my review.

bcolsen · 2018-10-13T08:01:35Z

I've been using this for the past couple of weeks and I ran into a problem with the traceback links. The code snipit shown in the traceback was showing the wrong line of code because the code line in the traceback was taken from the unsaved file and the code that was running was only in the editor. A solution to this would be to save the file before executing the code cell but that was one of the things we were trying to avoid.

So I decided to execute with the ipython run_cell() command instead of exec(compile()) which does all the things that notebook cells do including cell magic like %%timeit (which are syntax errors in normal python unfortunately). So this regression is fixed as well:

One other note on this implementation that could be seen as a regression.

Unlike the current method of running cells this method is unable to have the last line of the cell produce output in the IPython console.

And now the tracebacks don't have the right file name so the links don't work(I might be able to fix this) but the code snipit and the code lines are correct. So everything is now better than before.

bcolsen · 2018-10-13T09:41:08Z

Now the traceback links work too

bcolsen · 2018-10-14T05:40:36Z

I'm more happy about this now that the code cells are still executed as IPython cells and tracebacks links work. This is now better than before in every way unless you were editing cells in the qt console but I think that would be uncommon since editing in the editor is more convenient.

@ccordoba12 After the spyder kernel package is merged this should pass the tests (hopefully) and be ready for review.

ccordoba12

@bcolsen, this is really nice!! Thanks a lot for working on it!

About our tests: I'd also like to see a simple test that checks that our old method of cell evaluation works as expected (since the new method is going to become the default one).

ccordoba12 · 2018-10-14T15:53:33Z

spyder/plugins/editor/plugin.py

@@ -193,6 +193,8 @@ def setup_page(self):
        run_selection_group = QGroupBox(_("Run selection"))
        focus_box = newcb(_("Maintain focus in the Editor after running cells "
                            "or selections"), 'focus_to_editor')
+        run_cell_box = newcb(_("Use the run cell function to execute code "
+                               "cells"), 'run_cell_func')


Let's change the text of this checkbox to

Execute cells using a function instead of by pasting their contents to the console

@CAM-Gerlach, do you have a better suggestion?

A conservative edit would be Run code cells using a function instead of pasting their contents directly, but I'd favor something like Don't print cell contents when running code cells in the console for the label since it is more direct at naming the actual effect of the option for the end user (which may not be obvious from the previous) rather than an implementation detail. Either way, the tooltip could read Run code cells using the runcell() function in the console rather than pasting their full contents, for a cleaner console history.

The runcell() function is just the implementation of not having the full cell contents in the history and the console, so it likely doesn't need to be mentioned

For more clarity, I can flip the setting in the code and go for:

Copy full cell contents to the console when running code cells

With the setting unchecked by default.

The tool tip could read:

When running a code cell the full contents of the cell will be copied to the console and the history

The runcell() function is just the implementation of not having the full cell contents in the history and the console, so it likely doesn't need to be mentioned

Agreed, that's my opinion as well.

Copy full contents to the console when running code cells

I guess that could be a little clearer, although I don't feel that strongly a about it. As a minor refinement, I'd suggest Show full cell contents in the console when running code cells. While I'm normally very opposed to repetition, I think its necessary here for clarity. Regarding the tooltip, a suggested revision would be Print code cell contents to the Console and include them in the History Log when running cells.

I also agree with

Copy full cell contents to the console when running code cells

and making the option False. Thanks @bcolsen!

I think I'll go for:

Copy full cell contents to the console when running code cells

I like 'copy' in there because it's more than just shown or printed into the console, it's more like what would happen if you copied and pasted the cell into the console manually.

+1 from me.

spyder/plugins/editor/widgets/base.py

ccordoba12 · 2018-10-14T15:58:05Z

spyder/plugins/editor/widgets/editor.py

+        self.run_cell_func = state
+
+    def set_introspector(self, introspector):
+        self.introspector = introspector


This method (set_introspector) is not needed anymore (it was part of the old code completion architecture). Please remove it and merge your branch with master to have the latest changes to our tests.

Oops. This code is actually freshly rebased, but I just did a poor job on the manual conflict merge there

ccordoba12 · 2018-10-14T15:59:56Z

spyder/plugins/ipythonconsole/plugin.py

@@ -1025,6 +1026,61 @@ def run_script(self, filename, wdir, args, debug, post_mortem,
                  "<br><br>Please open a new one and try again."
                  ) % osp.basename(filename), QMessageBox.Ok)

+    def run_cell(self, code, cell_name, filename):


Ok, I'll try to refactor both methods here in your branch, after you address my review.

ccordoba12 · 2018-10-14T16:03:10Z

spyder/plugins/ipythonconsole/plugin.py

+        if client is None:
+            client = self.get_current_client()
+
+        is_internal_kernal = False


is_internal_kernal -> is_internal_kernel`

Also, we need to change this a little bit. runcell will also work in external Spyder kernels (i.e. kernels started with python -m spyder_kernels.console).

So we need to add a method to client that detects if kernels are of Spyder kind or not, and then avoid using runcell to run the code if they are not.

I think this might be best left for another PR (I can make the issue) since this is true for the run_script function above using runfile() instead of %run on internal consoles as well. The changes might as well be made at the some time.

I guess currently the run_script function wouldn't work too well on a remote machine unless the local files were mirrored in the remote machine. If this is the case one solution would be to run the contents of the whole file through runcell. Of course having real remote file editing would be better, but in the near term it could make remote kernels more friendly.

I think this might be best left for another PR (I can make the issue)

Ok, no problem. Please open the issue then, I'll take care of it.

CAM-Gerlach

I wasn't able to fully test it since the Spyder Kernels changes haven't yet gone through but I made note of some UI text, style and convention/best practice issues.

spyder/app/tests/script.py

CAM-Gerlach · 2018-10-14T18:50:28Z

spyder/plugins/editor/plugin.py

@@ -193,6 +193,8 @@ def setup_page(self):
        run_selection_group = QGroupBox(_("Run selection"))
        focus_box = newcb(_("Maintain focus in the Editor after running cells "
                            "or selections"), 'focus_to_editor')
+        run_cell_box = newcb(_("Use the run cell function to execute code "
+                               "cells"), 'run_cell_func')


A conservative edit would be Run code cells using a function instead of pasting their contents directly, but I'd favor something like Don't print cell contents when running code cells in the console for the label since it is more direct at naming the actual effect of the option for the end user (which may not be obvious from the previous) rather than an implementation detail. Either way, the tooltip could read Run code cells using the runcell() function in the console rather than pasting their full contents, for a cleaner console history.

CAM-Gerlach · 2018-10-14T18:53:23Z

spyder/plugins/editor/plugin.py

@@ -1464,6 +1469,9 @@ def register_editorstack(self, editorstack):
        editorstack.exec_in_extconsole.connect(
                                    lambda text, option:
                                    self.exec_in_extconsole.emit(text, option))
+        editorstack.run_cell_in_ipyclient.connect(
+                lambda code, cell_name, filename:


By PEP 8, a hanging indent should be 4 spaces, except for in function definitions. Spyder's automatic behavior (which I assume you relied on) is incorrect, which in fact prompted one of my very first issues here.

spyder/plugins/editor/widgets/base.py

CAM-Gerlach · 2018-10-14T18:55:22Z

spyder/plugins/editor/widgets/base.py

@@ -688,20 +690,25 @@ def get_selection_as_executable_code(self):
            else:
                lines.append(ls)

-        return ls.join(lines)
+        # Adding removed lines back to have correct traceback line numbers


Imperative tense ("Add removed lines...")

spyder/plugins/editor/widgets/editor.py

CAM-Gerlach · 2018-10-14T19:05:13Z

spyder/plugins/editor/widgets/editor.py

@@ -2426,10 +2433,31 @@ def advance_cell(self, reverse=False):
            term.setFocus()

    def re_run_last_cell(self):
-        text = self.get_current_editor().get_last_cell_as_executable_code()
+        text, line = self.get_current_editor().\


Per PEP 8 and our convention, we're using () for line continuations for all new code. So

text, line = (self.get_current_editor() .get_last_cell_as_executable_code())

CAM-Gerlach · 2018-10-14T19:08:13Z

spyder/plugins/editor/widgets/editor.py

+    def _run_cell_text(self, text, line):
+        """Run a cell text in the console
+
+        text(str): the text in the cell


Are you going for Googldoc here? If so, the correct format is:

Args: text (str): The text in the cell. line (int): The cell starting line.

CAM-Gerlach · 2018-10-14T19:08:16Z

spyder/plugins/editor/widgets/editor.py

+        self._run_cell_text(text, line)
+
+    def _run_cell_text(self, text, line):
+        """Run a cell text in the console


"Run a cell text"? Also, period.

CAM-Gerlach · 2018-10-14T19:08:46Z

spyder/plugins/ipythonconsole/plugin.py

@@ -914,6 +916,63 @@ def run_script(self, filename, wdir, args, debug, post_mortem,
                  "<br><br>Please open a new one and try again."
                  ) % osp.basename(filename), QMessageBox.Ok)

+    def run_cell(self, code, cell_name, filename):
+        """Run cell in current or dedicated client"""


bcolsen · 2018-10-17T06:31:05Z

I actually changed a bit of code in this latest commit so the copy cells go through the same code as the function cells. This way the copy cells and the function cells behave the same way with respect to dedicated cells (i.e. if there is a dedicated console for that file the code cells only run in that console.)

ccordoba12 · 2018-10-27T18:20:54Z

@bcolsen, please rebase in top of master to get the latest fixes to our tests.

@bcolsen and @CAM-Gerlach, it's really bad practice to discuss at length things unrelated to the main purpose of a PR (in this case why PyLS is failing on Windows) because it derails the discussion and impacts future reading of the PR contents.

This is something @CAM-Gerlach is fan of and I'm going to ask him to stop doing it. @CAM-Gerlach, for next time, please open a new issue or discuss things in our internal Gitter channel.

CAM-Gerlach · 2018-10-27T20:00:28Z

@ccordoba12 You're right, of course, we (and particularly I) got way off topic. My apologies for that. I posted it as a separate issue, #8153

bcolsen · 2018-10-27T20:27:19Z

@ccordoba12 Sorry for the noise on this thread.

Rebased.

I made some changes since the last review have the copy cells go through the same code as the function cells. This way the copy cells and the function cells behave the same way with respect to dedicated cells (i.e. if there is a dedicated console for that file the code cells only run in that console.)

ccordoba12

Great job @bcolsen! What a terrific improvement!!

bcolsen force-pushed the runcell branch from 775b3f5 to 4b0a2d6 Compare June 25, 2018 06:19

bcolsen mentioned this pull request Jun 25, 2018

PR: Add runcell to spydercustomize spyder-ide/spyder-kernels#7

Merged

7 tasks

CAM-Gerlach assigned bcolsen Jul 8, 2018

CAM-Gerlach added the type:Enhancement label Jul 8, 2018

CAM-Gerlach added this to the v4.0betaX milestone Aug 2, 2018

bcolsen mentioned this pull request Aug 24, 2018

Run unsaved file in Editor without needing to save #7760

Closed

10 tasks

ccordoba12 modified the milestones: v4.0betaX, v4.0beta2 Aug 24, 2018

bcolsen force-pushed the runcell branch from 4b0a2d6 to 3bfc82b Compare August 28, 2018 05:57

bcolsen commented Sep 8, 2018

View reviewed changes

bcolsen force-pushed the runcell branch from 6b8e9e5 to 19c1d7e Compare October 2, 2018 19:08

bcolsen force-pushed the runcell branch from 3c03299 to b02a348 Compare October 14, 2018 04:09

bcolsen changed the title ~~[WIP] Run cells in a function~~ Run cells in a function Oct 14, 2018

bcolsen changed the title ~~Run cells in a function~~ PR: Run cells in a function Oct 14, 2018

ccordoba12 reviewed Oct 14, 2018

View reviewed changes

CAM-Gerlach reviewed Oct 14, 2018

View reviewed changes

bcolsen added 22 commits October 27, 2018 14:08

runcell initial commit

5e916b2

fixed name and quotes

b462820

pep8

cca5710

Added an option in preferences to use the run_cell function

7220d60

run whole files, fix tests, fix lines numbers

72a8c45

cleaned up editor code and code cell names

8df92eb

pep8 cleanup

1c66baf

nomallize to function bug fix

2f36b78

Fix traceback links for code cells

5e6575b

pep8

cd5f431

test fix

a678127

Changed run_cell_func to run_cell_copy and documentation changes

ed7360c

pep8

7f52f25

Copy cell tests and all code cells to same exec path

3c5cd7d

pep8 and test fix

5d46c83

to_text_string

cea0a83

removed unicode from script.py

cbcfb0b

excape backslashes

6df4469

Test Changes

1dd735e

Change order of test assertations

c9a68d4

Added more wait time to test

f16516a

Added 500 ms of wait to tests

37989bc

bcolsen force-pushed the runcell branch from a661be6 to 37989bc Compare October 27, 2018 20:08

ccordoba12 approved these changes Oct 27, 2018

View reviewed changes

ccordoba12 changed the title ~~PR: Run cells in a function~~ PR: Run cells through a function instead of pasting their contents to the console Oct 27, 2018

ccordoba12 merged commit f0a2ea7 into spyder-ide:master Oct 27, 2018

jnsebgosselin mentioned this pull request Nov 14, 2018

Cannot runcell if double backslash is present #8241

Closed

2 tasks

PR: Run cells through a function instead of pasting their contents to the console #7310

PR: Run cells through a function instead of pasting their contents to the console #7310

Conversation

bcolsen commented Jun 19, 2018 • edited by ccordoba12 Loading

Pull Request Checklist

Description of Changes

Implementation

Issue(s) Resolved

pep8speaks commented Jun 19, 2018 • edited Loading

Comment last updated on October 27, 2018 at 20:09 Hours UTC

ccordoba12 commented Jun 23, 2018

bcolsen commented Jun 25, 2018

ccordoba12 commented Aug 24, 2018

ccordoba12 commented Aug 24, 2018

ccordoba12 commented Aug 29, 2018

bcolsen commented Aug 29, 2018

CAM-Gerlach commented Sep 4, 2018

bcolsen commented Sep 4, 2018

bcolsen commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcolsen commented Oct 13, 2018 • edited Loading

bcolsen commented Oct 13, 2018 • edited Loading

bcolsen commented Oct 14, 2018

ccordoba12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcolsen Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 Oct 14, 2018 • edited Loading

Choose a reason for hiding this comment

bcolsen Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CAM-Gerlach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcolsen commented Oct 17, 2018

ccordoba12 commented Oct 27, 2018

CAM-Gerlach commented Oct 27, 2018 • edited Loading

bcolsen commented Oct 27, 2018

ccordoba12 left a comment

Choose a reason for hiding this comment

bcolsen commented Jun 19, 2018 •

edited by ccordoba12

Loading

pep8speaks commented Jun 19, 2018 •

edited

Loading

bcolsen commented Oct 13, 2018 •

edited

Loading

bcolsen commented Oct 13, 2018 •

edited

Loading

bcolsen Oct 15, 2018 •

edited

Loading

ccordoba12 Oct 14, 2018 •

edited

Loading

bcolsen Oct 15, 2018 •

edited

Loading

CAM-Gerlach commented Oct 27, 2018 •

edited

Loading