PR: Allow empty wdir option to run_python_script_in_terminal

[jitseniesen]

Fixes #3155: Executing a python script in external terminal without
specifying a working directory leads to wdir = '', which is not handled
correctly.

In the previous version, the working directory was passed as an option
to the script if xterm is used. This did not make sense to me, so I
removed that.

[goanpeca]

@jitseniesen thanks for working on this. Could we add some tests :-) for this function.

[goanpeca]

Also since we are now dealing with wdir possibly being empty, should not we make it a kwargs?, Would this change be too disruptive (many places where this function is called?)

@Nodd, @ccordoba12 ?

---

Any other argument that might have a default value?

[jitseniesen]

@goanpeca I thought about adding tests, but I found it too hard :-) What this function does, depends on the OS and on whether certain programs are installed. Furthermore, the function actually runs a script. Should we depend on properties of the testing environment? Should we hide all external dependencies behind mocks?

Regarding making wdir a kwarg, I had a quick look and I think that's a good idea in principle. This function seems to be only called once, in MainWindow.open_external_console() in spyder.py. That function just forwards the wdir it is passed, so probably wdir should also be made a kwarg there. The function open_external_console() is used in two places, in Explorer and Editor, so just changing wdir to be a kwarg should be not too hard.

But if you start rewriting, where do you stop, and do we want to do this? I'm not familiar with the code, I don't find it very easy to read, it needs to handle Python2 and Python3 and different OSes so it needs to be careful with string encodings which I don't really know about, it's hard to test, and we are (as I understand it) close to releasing Spyder 3.0. All arguments (for me) to change the code as little as possible.

On having a further look, I wonder whether the windows branch has the same bug. And if we do decide to rewrite, it seems to me that as you suggested, other arguments should be given default values (all except fname, in fact). I also find it odd that WindowsError is caught in this function and displays a dialog box, but NotImplementedError (unsupported OS) is to be caught in the calling function, where it displays a dialog box; to me, this should be done in the same place.

[goanpeca]

Hi @jitseniesen yes it might be tricky to make changes right now, so caution at this point would be a good idea :-). Now for testing, in principle the tests folder could include some test script, either as an actual _script_func_test.py file, or as a text string inside the test_***.py

Basically it should be a simple script that has a verifiable output, so I think such script should just write something to a given location that the user provides via arguments and we should assert that that was the case. Since we run tests on appveyor and travis, we are testing on 2 OSs for now, eventually we will add OSX via travis (or split travis for OSX and circle CI for linux like conda-forge does).

The test script could receive a tmpfile and the content to check it worked, something like

script_test_external_console.py <tmpfile> <content>

where tmpfile is a pytest fixture and content is just whatever you want to write to check it worked :-)

What do you think?

[jitseniesen]

Thanks. I'll try to write a test.

[ccordoba12]

I think this looks good, thanks!