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

[REF] convenience commands for testing against a local kernel #425

Merged

Conversation

stevejpurves
Copy link
Collaborator

Problems this addresses:

  • Getting local development to work with a local kernel was not discoverable from the docs
  • switching to local changes required a code change to development.html, easy to commit in error

This PR aims to make it easier to chose to test against a local kernel without needing to change the code and improve discoverability.

Changes

  • moved single development.html to a folder and split to local.html and binder.html to cover the two cases
  • split out css to avoid repetition
  • added a note to the contributors guide
  • ensure previous npm run develop command works as before
  • added a npm run develop:local command to bring up the local page
  • added some instructions on how to start kernel on the local page

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This seems good to me 👍 thanks for updating the documentation as well, agree it is much easier to run this locally and iterate more quickly

can you get the lint test passing before merging though?

@moorepants
Copy link
Collaborator

LGTM too!

@stevejpurves
Copy link
Collaborator Author

thanks, I suggest we rebase this after getting #424 in which resolves the package-lock.json issue.

@stevejpurves stevejpurves changed the title convenience commands for testing against a local kernel [REF] convenience commands for testing against a local kernel Jul 20, 2021
@stevejpurves
Copy link
Collaborator Author

@moorepants @choldgraf The lint issue is ongoing but being dealt with see #426 , it would be great to get this PR in ahead of Monday if possible :D

@moorepants
Copy link
Collaborator

@TimStewartJ, It would be great if you try out this PR. You can get some review practice in and help out @stevejpurves. I can also try this out too.

@TimStewartJ
Copy link
Contributor

TimStewartJ commented Jul 27, 2021

Yep, tried it out and it looks good to me as well!

Edit with more relevant information:

I've messed around with both of the new HTML files and I agree that it allows for a much cleaner quick development environment. Appending and removing -disabled on the thebe configs in the old development.html got old pretty quick.

The new package scripts are a nice touch and well appreciated as well. I've tried them out and they work great.

@stevejpurves
Copy link
Collaborator Author

This one is now green ✅ on the lint stage too.
I resolved the conflicts with the updated master branch and this is ready to merge! 🥳

@moorepants moorepants merged commit f1ca548 into jupyter-book:master Aug 4, 2021
@stevejpurves stevejpurves deleted the feat/easier-local-kernel-dev branch October 4, 2021 08:58
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.

4 participants