Add binder preview on PR #46
Conversation
|
Screenshot of the last binder link:
|
| push: | ||
| branches: | ||
| - main |
There was a problem hiding this comment.
I'm not sure I understand this part. Does this say to run the workflow whenever a commit is pushed to the main branch?
There was a problem hiding this comment.
Yes! With that we will have a working demo that gets deployed in main 🤓
There was a problem hiding this comment.
In this case., we might want to add a badge in the README file to point to the demo, otherwise, people will not even know this exists unless they look at the PRs
|
I have not had time for a deep review - but just from a glance on the screenshot in the previous message I can see icons in black on top of the purple background... and I am going to go on a wild guess and mention
|
|
Question: is there a way to update the binder preview comment instead of creating a new comment on every commit? This will/might get annoying pretty soon, i.e. too many notifications and noise - especially when folks like me who send looooads of small commits in quick succession work on PRs |
I'm checking this issue and it is only happening in binder, I'll take a look why this is happening.
I'm going to see how to do it |
|
|
|
So apparently, the difference is that Binder is using JupyterLab verison 3.6.3 and my local environment was using 3.5.3. I'm not sure why, in the change of versions of 3.5.x to 3.6.x they decided to change the variable For now, I duplicated the variables, in the way that the extension will work with both and it is now working in binder as expected 🎉
|
Ok this raises another item to look at. Python 3.6 has reached its end of life and 3.7 is not far off see https://devguide.python.org/versions/. I would suggest then targeting Python >= 3.8 and drop anything below that |
There was a problem hiding this comment.
Thanks @steff456 - left a few comments and suggestions
In addition to the comments there:
- The
PostBuildscript is a Python script atm - is there a particular reason this was preferred over abashscript? Pure curiosity as I usually use the latter and find it easier and it helps me avoid calling subprocesses - The colours in the theme - did you use the ones in the
trallard/pitaya_smoothierepo or the Jupyterlab one? either is fine, but if you used the latter one I might need to tweak some colours (nothing bad nor for you to take on)
| push: | ||
| branches: | ||
| - main |
There was a problem hiding this comment.
In this case., we might want to add a badge in the README file to point to the demo, otherwise, people will not even know this exists unless they look at the PRs
| """ | ||
| print("\n\t", " ".join(args), "\n") | ||
| return_code = subprocess.call(args, **kwargs) | ||
| if return_code != 0: |
There was a problem hiding this comment.
would it not be best to raise a proper exception?
There was a problem hiding this comment.
This was part of the template I used. I think they do it to return the complete stacktrace into the console in case there's an error and not only the error.
|
Hi @trallard,
I'm using the template of binder for extensions and they are using a Python script instead of a bash one.. I don't have any preference but decided to take the template as it is.
I used the colors present in the JupyterLab extension repo, I'll create an issue to update the colors so they match the trallard/pitaya_smoothie repo. |
Apparently we already have a binder badge in the README on the main branch. It is currently broken, but it will work once we merge this PR.
|
This PR has the main objective of adding a preview in the PRs to check both installation and changes in a JupyterLab interface.
This PR,
environment.ymlandpostBuildAs this PR is setting an action the idea is to squash all the commits once it is working.