-
Notifications
You must be signed in to change notification settings - Fork 40
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
Migrate to Jupyter Lab v4 #135
base: master
Are you sure you want to change the base?
Conversation
js/package-lock.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that you are switching from npm
to yarn
we may want to use yarn
in the package.json
scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
thank you for the work!
I would prefer however if all unrelated changes are not pushed to this branch.
For ipyreact we added styling like this: widgetti/ipyreact#51
using ruff + prettier but that should not go into this PR.
Also, does a build with jupyterlab4 make ipywebrtc still work in jupyterlab3.x?
I'm happy to have formatting/style fixes btw, but not mixed with other PR's :)
Hope I don't discouraged you too much, feel free to push back.
Cheers,
Maarten
.github/workflows/main.yml
Outdated
- name: Checkout | ||
uses: actions/checkout@v2 | ||
|
||
- name: Install Conda environment with Micromamba | ||
uses: mamba-org/setup-micromamba@v1 | ||
with: | ||
environment-file: dev_environment.yml | ||
condarc: | | ||
dependencies: | ||
- python==${{ matrix.python-version }} | ||
|
||
- name: Test flake8 | ||
run: flake8 ipywebrtc --ignore=E501,F405 | ||
|
||
- name: Install ipywebrtc | ||
run: pip install . | ||
|
||
- name: Check installation files | ||
run: | | ||
test -d $CONDA_PREFIX/share/jupyter/nbextensions/jupyter-webrtc | ||
test -f $CONDA_PREFIX/share/jupyter/nbextensions/jupyter-webrtc/extension.js | ||
test -f $CONDA_PREFIX/share/jupyter/nbextensions/jupyter-webrtc/index.js | ||
test -d $CONDA_PREFIX/share/jupyter/labextensions/jupyter-webrtc | ||
test -f $CONDA_PREFIX/share/jupyter/labextensions/jupyter-webrtc/package.json | ||
|
||
- name: Check nbextension and labextension | ||
run: | | ||
jupyter labextension list 2>&1 | grep -ie "jupyter-webrtc.*enabled.*ok" - | ||
|
||
- name: Run js tests | ||
run: | | ||
npm install | ||
npm run test | ||
working-directory: js | ||
|
||
- name: Build docs (Only on MacOS for build speed) | ||
if: matrix.os == 'macos-latest' | ||
run: | | ||
cd docs/source/ | ||
sphinx-build . _build/html | ||
cd ../.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid unneeded changes/reformatting, or put them in a separate PR or top commit first. This makes tracking changes much easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what you're saying, I can do a new PR for style changes.
- name: Checkout | ||
uses: actions/checkout@v2 | ||
|
||
- name: Checkout | ||
uses: actions/checkout@v2 | ||
|
||
- name: Install Conda environment with Micromamba | ||
uses: mamba-org/provision-with-micromamba@main | ||
with: | ||
environment-name: ipywebrtc-dev | ||
environment-file: dev_environment.yml | ||
python-version: ${{ matrix.python-version }} | ||
mamba-version: "*" | ||
auto-activate-base: false | ||
channels: conda-forge | ||
|
||
- name: Build packages | ||
run: | | ||
python setup.py sdist bdist_wheel | ||
cd dist | ||
sha256sum * | tee SHA256SUMS | ||
- name: Upload builds | ||
uses: actions/upload-artifact@v2 | ||
with: | ||
name: dist ${{ github.run_number }} | ||
path: ./dist | ||
- name: Install Conda environment with Micromamba | ||
uses: mamba-org/setup-micromamba@v1 | ||
with: | ||
environment-file: dev_environment.yml | ||
condarc: | | ||
dependencies: | ||
- python==${{ matrix.python-version }} | ||
|
||
- name: Build packages | ||
run: | | ||
python setup.py sdist bdist_wheel | ||
cd dist | ||
sha256sum * | tee SHA256SUMS | ||
- name: Upload builds | ||
uses: actions/upload-artifact@v2 | ||
with: | ||
name: dist ${{ github.run_number }} | ||
path: ./dist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, very difficult to see what actually changed and what is related to the intention or the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easier to review hiding the whitespaces changes https://github.com/maartenbreddels/ipywebrtc/pull/135/files?diff=split&w=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
w=1
is the query parameter
- python: "3.6" | ||
dist: "ipywebrtc*.tar.gz" | ||
- python: "3.9" | ||
dist: "ipywebrtc*.whl" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't that bad, because it's not mixed with other changes, but adds noise.
import * as services from "@jupyterlab/services"; | ||
import * as Backbone from "backbone"; | ||
import * as widgets from "@jupyter-widgets/controls"; | ||
import * as base from "@jupyter-widgets/base"; | ||
import * as sinon from "sinon"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the changes in this file are not needed, although I welcome a style fix :)
No description provided.