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

docs!: switch to MyST #208

Merged
merged 15 commits into from
Jul 26, 2021
Merged

docs!: switch to MyST #208

merged 15 commits into from
Jul 26, 2021

Conversation

redeboer
Copy link
Contributor

@redeboer redeboer commented Jul 24, 2021

Closes #206

Warning: Notebooks are now rendered with myst-nb, but this has no equivalent to nbsphinx_prolog, so the prologue message is not visible in the example pages anymore.

image

A solution would be to switch to the sphinx-book-theme for its launch buttons. That requires some more modifications though, so I split that into a follow-up PR (#209).

Some related improvements:

redeboer added 6 commits July 24, 2021 19:49
Also use the correct package names from PyPI (like dashes instead of
underscores)
WARNING: this removes the nbsphinx_prolog on the top on each example
page, as there is no equivalent of that in myst-nb
https://nbsphinx.readthedocs.io/en/0.8.6/prolog-and-epilog.html
@redeboer
Copy link
Contributor Author

Not sure why pre-commit.ci does not autofix the style issues...

@redeboer
Copy link
Contributor Author

@ianhi another small doc related thing: could you abbreviate the link to the docs on the about section for the repo to https://mpl-interactions.rtfd.io (excluding the final slash)?

Copy link
Collaborator

@ianhi ianhi left a comment

Choose a reason for hiding this comment

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

This is fantastic thank you for all the work! and for the excellent PR/commit messages/explanations! They make it much easier to follow.

@ianhi another small doc related thing: could you abbreviate the link to the docs on the about section for the repo to https://mpl-interactions.rtfd.io (excluding the final slash)?

Sure thing. What benefit does this bring?

@@ -124,6 +124,10 @@
"python": ("https://docs.python.org/3", None),
}

# Settings for copybutton
copybutton_prompt_is_regexp = True
copybutton_prompt_text = r">>> |\.\.\. " # doctest
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this actually come up anywhere? Why not include the full regex including the [In] etc to protect against that in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point, there are currently no doctests. But still, once there are (and I encourage adding them, because they can be tested with pytest as well), these lines make the copy button there more convenient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to add the --doctest-modules flag to pytest.ini, but then noticed this 'bug' #210.

Copy link
Collaborator

Choose a reason for hiding this comment

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

doctest = testing snippets in docstrings and in .md and .rst files? The notebooks that make up the majority of the docs are already included in the test runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the ones in md and rst files would be doctests too, but I initially only had doctests in the docstrings (that is, the API) in mind. It's in line with #203, where the idea is that the API can serve more as its own documentation as well (with some integrated examples). That said, I now start to wonder how useful doctests are for a matplotlib framework..
Let's move this to #208.

@ianhi
Copy link
Collaborator

ianhi commented Jul 26, 2021

Not sure why pre-commit.ci does not autofix the style issues...

I'll look at this tomorrow. After that are there any other changes you want here?

@redeboer
Copy link
Contributor Author

Sure thing. What benefit does this bring?

Aesthetics 🙌 😉

@ianhi
Copy link
Collaborator

ianhi commented Jul 26, 2021

Not sure why pre-commit.ci does not autofix the style issues...

I'll look at this tomorrow. After that are there any other changes you want here?

It seems that prettier insists on modifying staged files which pre-commit is not ok with. I can replicate the failure locally so it's about the interaction of pre-commit and prettier and I wasn't able to find an easy way around this. I'm ok with modifying those files manually and having pre-commit ci fail for md etc

https://stackoverflow.com/a/64309843/835607

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ianhi
Copy link
Collaborator

ianhi commented Jul 26, 2021

Fantastic thank you!


@all-contributors please add @redeboer for documentation

@ianhi ianhi merged commit d5dffd7 into mpl-extensions:master Jul 26, 2021
@allcontributors
Copy link
Contributor

@ianhi

I've put up a pull request to add @redeboer! 🎉

@redeboer redeboer deleted the myst branch July 26, 2021 18:34
@redeboer
Copy link
Contributor Author

Not sure why pre-commit.ci does not autofix the style issues...

I'll look at this tomorrow. After that are there any other changes you want here?

It seems that prettier insists on modifying staged files which pre-commit is not ok with. I can replicate the failure locally so it's about the interaction of pre-commit and prettier and I wasn't able to find an easy way around this. I'm ok with modifying those files manually and having pre-commit ci fail for md etc

https://stackoverflow.com/a/64309843/835607

I see, thanks for looking into this. I don't get though why autofix (which assumingly involves re-staging) did work for black and isort.

@redeboer
Copy link
Contributor Author

Autofix also works for nbqa, it seems: a7885a1. So I guess it's something with Prettier (which is node.js)?

@ianhi
Copy link
Collaborator

ianhi commented Jul 26, 2021

I see, thanks for looking into this. I don't get though why autofix (which assumingly involves re-staging

I think in this case pre-commit ci is handling the git commands, whereas prettier (I think?) essentially is doing something like git commit --amend which is antithetical to how pre-commit operates

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.

Switch Docs to MYST
2 participants