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

👌 Fix myst-parser install issues #599

Closed
wants to merge 8 commits into from

Conversation

chrisjsewell
Copy link
Contributor

@chrisjsewell chrisjsewell commented Aug 30, 2020

This commit removes myst-parser as an optional dependency, and instead directly uses markdown-it-py for parsing, adding it as a dependency.

Note: this addition drops python support <= 3.5, but this is in-line with the Python End-of-life guiidance: https://devguide.python.org/#status-of-python-branches

closes #591

This commit removes myst-parser as an optional dependency, and instead directly uses markdown-it-py for parsing, adding it as a dependency.

Note: this addition drops python support <= 3.5, but this is in-line with the Python End-of-life guiidance: https://devguide.python.org/#status-of-python-branches
@chrisjsewell
Copy link
Contributor Author

I've also made pip CI run on PRs. Surely this is a good idea?

@codecov
Copy link

codecov bot commented Aug 30, 2020

Codecov Report

Merging #599 into master will decrease coverage by 0.22%.
The diff coverage is 92.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
- Coverage   99.00%   98.77%   -0.23%     
==========================================
  Files          90       90              
  Lines        8951     8927      -24     
==========================================
- Hits         8862     8818      -44     
- Misses         89      109      +20     
Impacted Files Coverage Δ
tests/test_cli.py 100.00% <ø> (ø)
tests/test_ipynb_to_myst.py 100.00% <ø> (ø)
tests/test_mirror.py 100.00% <ø> (ø)
tests/utils.py 100.00% <ø> (ø)
jupytext/myst.py 96.58% <91.66%> (-0.83%) ⬇️
jupytext/formats.py 96.60% <100.00%> (ø)
tests/test_formats.py 100.00% <100.00%> (ø)
tests/test_using_cli.py 100.00% <100.00%> (ø)
jupytext/reraise.py 25.00% <0.00%> (-50.00%) ⬇️
tests/test_read_simple_pandoc.py 89.47% <0.00%> (-10.53%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b192d5b...5de34a1. Read the comment docs.

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Aug 30, 2020

you could also consider adding a pre-commit check to the CI:

jobs:
  pre-commit:

    runs-on: ubuntu-latest

    steps:
    - uses: actions/checkout@v2
    - name: Set up Python 3.8
      uses: actions/setup-python@v1
      with:
        python-version: 3.8
    - uses: pre-commit/[email protected]

(and a pypa/[email protected] job)

@mwouts
Copy link
Owner

mwouts commented Aug 30, 2020

Hello @chrisjsewell , thank you very much for this!

I am a little bit worried of dropping support for older versions of Python, as I see that about 15% of jupytext downloads are still for Python 2.7, and 7% for Python 3.5 (and, the absence of support for older versions of Python was one issue that drove me away from notedown and ipymd two years ago...)

Do you think we could

  • require markdown-it-py unconditionally both in requirements.txt and for the conda package
  • but require markdown-it-py in setup.py only in Python 3.6 and above?

you could also consider adding a pre-commit check to the CI

Yes, that's a great idea!

I've also made pip CI run on PRs. Surely this is a good idea?

Well, currently, pushing to a branch of https://github.com/mwouts/jupytext triggers the CI, so I can see the state of the CI before making the PR. Doesn't this work for you?

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Aug 30, 2020

I am a little bit worried of dropping support for older versions of Python

Eurggh, well obviously its entirely up to you 😄
But I would advise against it; when Python drops support for a version then it should no longer be used. Jupyter notebook itself dropped python 2.7 support long ago and will drop 3.5 in 6.1 (jupyter/notebook#5538).
I imagine most of these people would be pushed to upgrade their pythons, and off course they can always use earlier versions of jupytext.

But yeh let me know, and otherwise I guess we'll have to go through the hassle of adding back all these missing install checks and pytest markers?

(Note also, if this is the case, we would be able to consider using markdown-it-py, to fix the current issue with variable length ` code fences)

Doesn't this work for you?

The person making the PR cannot see that though 😬, its quite unusual
The trigger setup I've added here is one we use for all packages in executablebooks/aiida, and we have never had any problems.
But again I can remove it if you wish? 😄

@chrisjsewell
Copy link
Contributor Author

I am a little bit worried of dropping support for older versions of Python

and f-strings, how can you not want to use f-strings 😆

@mwouts
Copy link
Owner

mwouts commented Aug 30, 2020

Thanks Chris for your understanding

I guess we'll have to go through the hassle of adding back all these missing install checks and pytest markers?

I'll do that, no worries! Maybe I'll push that to another branch first, anyway I'll keep you posted.

The person making the PR cannot see that though, its quite unusual

Understood!

and f-strings, how can you not want to use f-strings 😆

Oh that's right, I love f-strings, and I do use them all the time in my day work... just not here!

@chrisjsewell
Copy link
Contributor Author

I'll do that, no worries! Maybe I'll push that to another branch first, anyway I'll keep you posted.

Cheers 👍

@mwouts
Copy link
Owner

mwouts commented Aug 30, 2020

@chrisjsewell , I have prepared #601. Is it fine with you?

@mwouts mwouts mentioned this pull request Aug 30, 2020
5 tasks
@mwouts
Copy link
Owner

mwouts commented Aug 30, 2020

Integrated at #601

@mwouts mwouts closed this Aug 30, 2020
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.

Upgrade myst-parser dependency to 0.12.x
2 participants