-
Notifications
You must be signed in to change notification settings - Fork 18
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
Split CI into build, lint, test #32
Conversation
Codecov Report
@@ Coverage Diff @@
## master #32 +/- ##
=========================================
Coverage ? 89.85%
=========================================
Files ? 13
Lines ? 936
Branches ? 0
=========================================
Hits ? 841
Misses ? 95
Partials ? 0 Continue to review full report at Codecov.
|
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.
Thanks @bollwyvl
I have two questions otherwise this is great.
I'll rebase the port to JLab3 once this is merged.
.github/workflows/build.yml
Outdated
- name: Install JS Dependencies | ||
if: ${{ steps.cache-node-modules.outputs.cache-hit != 'true' }} | ||
run: jlpm --prefer-offline --frozen-lockfile | ||
|
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.
Is there a reason to install JS dependencies once more (already in l154)?
- name: Install JS Dependencies | |
if: ${{ steps.cache-node-modules.outputs.cache-hit != 'true' }} | |
run: jlpm --prefer-offline --frozen-lockfile | |
- name: Install JS Dependencies | |
if: ${{ steps.cache-node-modules.outputs.cache-hit != 'true' }} | |
run: jlpm --prefer-offline --frozen-lockfile | |
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.
yup!
setup.cfg
Outdated
name = jupyterlab_pullrequests | ||
version = attr: jupyterlab_pullrequests._version.__version__ | ||
description = Pull Requests for JupyterLab | ||
python_requires = >=3.6 |
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.
python_requires = >=3.6 |
The correct one is in [options]
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.
cleaned up, thanks!
I am happy to help on that, but yeah, let's get it merged first. On |
Thanks for the quick correction.
Agree it is not of much help. I let you decide if you want to drop it now or as part of #31 Feel free to merge and if you want to update #29 be my guest 😃 I'm working on JS testing - restoring the existing one at least. |
I'll merge this, as-is, and make a sub-pr against #29 so you're not fighting too many beasts at once! |
This adds a couple jobs, and coverage on more python/os/nodes.
My self-PR with all the changes, though in fighting CI, I mostly force-pushed, so this diff is as readable as it's going to get.
Some notable deltas (aside from the main workflow file change):
jupyter_packaging
(andpyproject.toml
altogether)jlpm
andjlpm build
, which seems.... dubious.