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

⬆️ UPGRADE: myst-parser to 0.15.2 #353

Merged
merged 5 commits into from
Aug 29, 2021
Merged

⬆️ UPGRADE: myst-parser to 0.15.2 #353

merged 5 commits into from
Aug 29, 2021

Conversation

mmcky
Copy link
Member

@mmcky mmcky commented Aug 25, 2021

This PR upgrades myst-parser to version 0.15.2.

This includes the following improvements and changes

Tasks

  • Wait for myst-parser 0.15.2 release
  • Update dependency here
  • Merge!

@mmcky mmcky mentioned this pull request Aug 25, 2021
4 tasks
@chrisjsewell
Copy link
Member

Thanks @mmcky! but I think you are missing the crucial change of actually updating setup.cfg lol

@chrisjsewell
Copy link
Member

ah yeh thats the one

@mmcky
Copy link
Member Author

mmcky commented Aug 25, 2021

Thanks @mmcky! but I think you are missing the crucial change of actually updating setup.cfg lol

Yeah thanks -- I used git checkout . when working with the test set and it reset the change :-(. All fixed up.

@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #353 (be185b2) into master (b132a50) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #353   +/-   ##
=======================================
  Coverage   87.43%   87.43%           
=======================================
  Files          12       12           
  Lines        1337     1337           
=======================================
  Hits         1169     1169           
  Misses        168      168           
Flag Coverage Δ
pytests 87.43% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 b132a50...be185b2. Read the comment docs.

@mmcky
Copy link
Member Author

mmcky commented Aug 25, 2021

  • update readthedocs and fix this warning
WARNING: The config value `myst_url_schemes' has type `tuple', defaults to `list'.

@choldgraf
Copy link
Member

ah excellent! @mmcky is this ready for review or do you wanna get tests passing first?

@mmcky
Copy link
Member Author

mmcky commented Aug 26, 2021

thanks @choldgraf I think I just fixed the readthedocs issue. So I'll tag this for review once everything is green.

@mmcky
Copy link
Member Author

mmcky commented Aug 26, 2021

Thanks @choldgraf and @chrisjsewell I think this is ready to 🚢

@chrisjsewell
Copy link
Member

WARNING: The config value myst_url_schemes' has type tuple', defaults to `list'.

Hmm, maybe this should be "fixed" in myst-parser, as really there should be no problem with a tuple (you can specify a list of allowed types https://www.sphinx-doc.org/en/master/extdev/appapi.html#sphinx.application.Sphinx.add_config_value)

@chrisjsewell
Copy link
Member

Thanks @choldgraf and @chrisjsewell I think this is ready to 🚢

Maybe just wait for me to release v0.15.2 and pin to that, since it is anyway needed for sphinx 4: #352 (comment). Will try to do in next day or 2

@choldgraf
Copy link
Member

sounds good - I have added the next steps in this PR...we should get it merged quickly once 0.15.2 is out so that we can bump up the dependencies in jupyter book and resolve some of these pipfile issues 😬

@mmcky
Copy link
Member Author

mmcky commented Aug 27, 2021

WARNING: The config value myst_url_schemes' has type tuple', defaults to `list'.

Hmm, maybe this should be "fixed" in myst-parser, as really there should be no problem with a tuple (you can specify a list of allowed types https://www.sphinx-doc.org/en/master/extdev/appapi.html#sphinx.application.Sphinx.add_config_value)

@chrisjsewell yeah I thought that might be true but it was actually specified in docs/conf.py of this repo so this is fixed now.

Ah I just re-read your comment -- I see. So a tuple should be OK.

setup.cfg Outdated Show resolved Hide resolved
@choldgraf choldgraf changed the title ⬆️ UPGRADE: myst-parser to 0.15.1 ⬆️ UPGRADE: myst-parser to 0.15.2 Aug 27, 2021
choldgraf
choldgraf previously approved these changes Aug 27, 2021
Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I've bumped to 0.15.2.

The changes all look good to me - seem pretty straightforward. I had one question about person-specific paths in the regression files, but I don't think that needs to block this since those will be manually updated in future PRs anyway

@@ -430,7 +430,7 @@
"execution_count": 5,
"metadata": {
"filenames": {
"image/png": "/private/var/folders/t2/xbl15_3n4tsb1vr_ccmmtmbr0000gn/T/pytest-of-chrisjsewell/pytest-679/test_complex_outputs_unrun_aut0/source/_build/jupyter_execute/complex_outputs_unrun_22_0.png"
"image/png": "/private/var/folders/_w/bsp9j6414gs4gdlnhhcnqm9c0000gn/T/pytest-of-matthewmckay/pytest-37/test_complex_outputs_unrun_aut0/source/_build/jupyter_execute/complex_outputs_unrun_22_0.png"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any way we can configure tests to not be person-specific? otherwise I think this will be re-written any time somebody has to update the tests who didn't update them in the past.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@choldgraf I just updated a whole bunch of metadata in the tests in upgrade to sphinx 4 and the tests actually ignore the metadata

set_notebook_diff_targets(metadata=False)

I don't see an easy way to generate these fixtures with generic values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned, the actual tests ignore these values when diffing.
if you wanted to reduce the diff though, it would be possible to add a pre-commit hook, which replaces all the "non-deterministic" content with deterministic ones

@mmcky
Copy link
Member Author

mmcky commented Aug 29, 2021

I think this is good to go -- do you agree @chrisjsewell?

Once this is merged I will start on sphinx >= 4 work and will rebase on this starting point.

docs/conf.py Outdated Show resolved Hide resolved
Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cheers!

@mmcky
Copy link
Member Author

mmcky commented Aug 29, 2021

thanks @chrisjsewell 👍 once all tests reporting green -- I'll merge.

@mmcky mmcky merged commit a3b2c6e into master Aug 29, 2021
@mmcky mmcky deleted the myst-parser=0.15.1 branch August 29, 2021 23:38
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.

3 participants