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

adding cwd param to terminal api endpoint #160

Closed
wants to merge 7 commits into from
Closed

adding cwd param to terminal api endpoint #160

wants to merge 7 commits into from

Conversation

qntnrbns
Copy link
Contributor

@qntnrbns qntnrbns commented Dec 18, 2019

I am trying to fix this issue jupyterlab/jupyterlab#1366 in jupyterlab so that terminals open in the current working directory.

I am bumping the version of terminado to 0.8.3 to capture jupyter/terminado#70, and adding a json param to the terminal creation endpoint.

I want to write a test for this, but could use some guidance w.r.t. the new test suite and how much we need to mock the creation of the terminal

Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @qntnrbns! Let a few comments inline.

jupyter_server/terminal/api_handlers.py Outdated Show resolved Hide resolved
jupyter_server/terminal/api_handlers.py Outdated Show resolved Hide resolved
@Zsailer
Copy link
Member

Zsailer commented Dec 19, 2019

Great work writing the tests @qntnrbns! Thanks for working on that.

I'm not sure why the Windows tests are hanging. I'll try to look into this later today.

Before that, I'm putting together a minor release (0.2.0) today—if your PR doesn't get in for that... we'll put a patch release out right after with this PR 🎉

@qntnrbns
Copy link
Contributor Author

@Zsailer were you able to fix the windows build?

tests/test_terminal.py Outdated Show resolved Hide resolved
@Zsailer
Copy link
Member

Zsailer commented Jan 14, 2020

@qntnrbns We likely need to make these tests conditional on the running platform. The same terminal params that work on Linux/MacOS won't work on Windows.

There are two options for this PR:

  1. We just skip these tests on Windows.
  2. Or we write tests with separate terminal parameters for each platform.

I'm fine with either choice at this point.

Note that we've ripped out the conftest.py file and make it a pytest_plugin. Would you like to try updating your PR? Or I'm happy to move this forward if you're limited on time.

@qntnrbns
Copy link
Contributor Author

I took a shot at it, but I don't have a windows machine

@blink1073
Copy link
Contributor

Given that terminado itself skips all but the basic tests on Windows due to timeouts, I think it is fair to just verify that we can submit the kwargs on Windows and not wait for a response.

@Zsailer
Copy link
Member

Zsailer commented Apr 10, 2020

Closing in favor of #201 (I was having issues pushing changes directly to this branch).

Thanks for this feature, @qntnrbns!

@Zsailer Zsailer closed this Apr 10, 2020
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.

3 participants