-
Notifications
You must be signed in to change notification settings - Fork 393
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
Add mystnb format #456
Add mystnb format #456
Conversation
Hmm, all tests are passing for the required python versions, but InvalidVersionSpec: Invalid version '0.8;python_version>='3.6'': invalid character(s) Sure you don't want to just drop python 2.7 & 3.4 in this PR 😉 |
Codecov Report
@@ Coverage Diff @@
## master #456 +/- ##
========================================
Coverage 98.91% 98.91%
========================================
Files 77 79 +2
Lines 7563 7762 +199
========================================
+ Hits 7481 7678 +197
- Misses 82 84 +2
Continue to review full report at Codecov.
|
@@ -39,7 +39,7 @@ install: | |||
pip install -r requirements.txt; | |||
fi | |||
# install is required for testing the pre-commit mode | |||
- pip install . || true | |||
- pip install .[myst] || true |
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 moved the install to an extra, if thats ok
@choldgraf you'll like this one; in d627398 I have introduced logic to compact the metadata blocks. For blocks with no nested dicts, the block is denoted by starting colons::
For blocks with nesting the block is enlosed by
(these two representations are synonymous in the MyST spec: https://myst-parser.readthedocs.io/en/latest/using/syntax.html#parameterizing-directives) |
Hello @chrisjsewell , that's awesome! Sorry to have caused you trouble with Python 2.7, I prefer to keep testing it, but I agree, it's hard to make the CI work with conditional dependencies. Adding the extra dependencies in the CI script itself is perfectly OK. Anyway, maybe I should rewrite the part of Tonight I'll take the time to review this with more detail. We could ship that soon, at least if it's OK on your side. Maybe we could
Do you see anything else that you'd like to add? |
Oh yeh, should have probably read that lol!
I've added
No, I think that should just about cover it 👍 |
A markdown cell | ||
And below, the cell for function f has non trivial cell metadata. And the next cell as well. | ||
|
||
```{nb-code} ipython3 |
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.
Isn't this planned to be {execute}
?
This looks great to me - with one main question, which is that I see a few discrepancies between the executablebooks/MyST-NB#12 (comment) In particular, using Is there a reason for the change? re: |
This is the 'tricky' part; currently
If you wanted this to be directly used by
This then follows on from the above; with this "
In this context, |
FYI I had a few difficulties following the |
I added MyST to the jupyterlab extension, but I'm having trouble installing it to test. So I've put the commit it in a separate branch for now a6a95e8 might need you to add that for me @mwouts FYI I note that in the package.json, the jupyterlab dependancy is set to version 1, but now version 2 is released. I'm guessing that's on the TODO list to update |
That all sounds good to me @chrisjsewell - @mwouts if we end up changing the names of some things then we are happy to upstream a PR to keep jupytext up-to-date with things. |
Added mystnb to benchmarking (4c206ec) and no problems there (faster than |
Impressive work @chrisjsewell ! I'll merge this now, and then maybe we can do a bit more testing, and ship this in a few days... A few comments:
|
In a few more minutes I would of committed that 😆
Ah oops, that must have been down to way I fork it off of the ExecutableBookProject:master
Cheers 😄 |
@chrisjsewell , if you like you're actually very close to having a PR for this... your change is correct, but should be rebased to match the latest version of the Lab extension (and version number could thus be 1.2.1). Just one detail - could you also update And if you want to try the lab extension locally, I recommend building ( |
Heya @mwouts the new format we have discussed for you to look at 😄
closes #447, originally implemented in executablebooks/MyST-NB#82
See https://myst-parser.readthedocs.io and https://myst-nb.readthedocs.io for further details on the format. The file extension is still up for discussion. Also, at present, I copy
nbformat
andnbformat_minor
to the front matter, but that's probably not necessary?All tests pass, apart from 3 that are skipped due to diffs in newline characters at the end of markdown blocks, which don't actually affect the consistency of the round-trip.
cc'ing @choldgraf @AakashGfude @mmcky @jstac