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

Replace recommonmark with myst (docs) #1021

Merged
merged 10 commits into from
Jun 27, 2020
Merged

Replace recommonmark with myst (docs) #1021

merged 10 commits into from
Jun 27, 2020

Conversation

luizirber
Copy link
Member

@luizirber luizirber commented Jun 12, 2020

MyST is a new markdown parser that supports features from ReStructureText, especially Sphinx directives and roles. This might be enough to replace the last .rst document (the index), for example, while also allowing more customization lost on the rst -> md transition.

TODO

Checklist

  • Is it mergeable?
  • make test Did it pass the tests?
  • make coverage Is the new code covered?
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Was a spellchecker run on the source code and documentation after
    changes were made?

@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #1021 into master will increase coverage by 8.75%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1021      +/-   ##
==========================================
+ Coverage   83.30%   92.05%   +8.75%     
==========================================
  Files          97       72      -25     
  Lines        8749     5453    -3296     
==========================================
- Hits         7288     5020    -2268     
+ Misses       1461      433    -1028     
Flag Coverage Δ
#rusttests ?
Impacted Files Coverage Δ
sourmash/_compat.py 52.17% <0.00%> (-47.83%) ⬇️
sourmash/signature.py 88.42% <0.00%> (-2.32%) ⬇️
sourmash/lca/lca_utils.py 93.75% <0.00%> (-2.09%) ⬇️
sourmash/lca/command_index.py 89.83% <0.00%> (-0.54%) ⬇️
sourmash/sbt.py 87.32% <0.00%> (-0.28%) ⬇️
src/core/tests/signature.rs
src/core/src/index/sbt/mod.rs
src/core/src/ffi/minhash.rs
src/core/src/cmd.rs
src/core/tests/test.rs
... and 21 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 bc2b168...ada128e. Read the comment docs.

@ctb
Copy link
Contributor

ctb commented Jun 13, 2020

question! does it flag missing local references for you, or are you just being extra careful in your review?

@luizirber
Copy link
Member Author

question! does it flag missing local references for you, or are you just being extra careful in your review?

A bit of both. I replaced full URLs (pointing to sourmash.readthedocs.org), which allows finding internal references (and where they are linking to anchors that don't exist anymore). I also removed the recommendation to install pre-releases of sourmash 2.0 (!!!), and fixed some notebooks that had multiple <h1>-level headings (sphinx doesn't like that).

There are still many warnings about documents not properly linked in toctrees, I think it's useful to fix them and then use sphinx-build -W (turn warnings into errors) to avoid having any warnings pop up again.

@luizirber
Copy link
Member Author

heh, the myst parser doesn't support Python 2.7, so this broke Travis =P

@luizirber luizirber changed the title [WIP] Replace recommonmark with myst (docs) Replace recommonmark with myst (docs) Jun 13, 2020
@luizirber
Copy link
Member Author

Ready for review and merge @ctb @olgabot

@ctb
Copy link
Contributor

ctb commented Jun 14, 2020

I'm not immediately sure why this is happening, but make doc no longer builds the docs on this branch, despite its presence in the Makefile.

@ctb
Copy link
Contributor

ctb commented Jun 14, 2020

oh, I see the diff... but that doesn't work for me :)

@ctb
Copy link
Contributor

ctb commented Jun 14, 2020

I built it locally!

Questions:

  • Do we want to add -W in this PR, or make an issue and do it in another PR? It looks like it works for me on a clean checkout.
  • Has this been built successfully on RTD?

I haven't reviewed the content changes yet, will get into that next.

Copy link
Contributor

@ctb ctb left a comment

Choose a reason for hiding this comment

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

Looks great, overall! A few minor questions and comments.

@@ -205,3 +205,14 @@ For the Rust core library we use `rMAJOR.MINOR.PATH`
(note it starts with `r`, and not `v`).
The Rust version is not automated,
and must be bumped in `src/core/Cargo.toml`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this TOC ends up at the bottom of the file. Is this intentional (and if so, why 😂)

Copy link
Member Author

Choose a reason for hiding this comment

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

I collected orphan links into a toc, but opted to put it in the bottom to avoid disrupting the current docs. But it might be more useful to put on the top...

doc/other-languages.md Outdated Show resolved Hide resolved
doc/other-languages.md Outdated Show resolved Hide resolved
doc/release-notes/releases.md Show resolved Hide resolved
Copy link
Collaborator

@olgabot olgabot 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 awesome! My only comment is small and nitpicky. Some places have docs plural and some have doc singular. For clarity, is it possible to have only one spelling?

@ctb
Copy link
Contributor

ctb commented Jun 16, 2020

@olgabot re

Some places have docs plural and some have doc singular. For clarity, is it possible to have only one spelling?

I did some grepping in the markdown and can't find places where doc, singular, was used -- could you point us at a specific example? I'm on board with picking one just want to make sure I understand :)

Copy link
Collaborator

@olgabot olgabot left a comment

Choose a reason for hiding this comment

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

Turns out all the mentions of docs were in the Travis and tox files! The documentation folder is called doc singular, so maybe that's the way to go? Maybe less of a deal than I thought initially, but it tripped me up on my first read.

- <<: *test
python: 3.7
end:
- TOXENV=docs
Copy link
Collaborator

Choose a reason for hiding this comment

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

docs plural

Makefile Outdated
@@ -23,7 +23,7 @@ test: all
$(PYTHON) -m pytest
cargo test

doc: .PHONY
doc: build
Copy link
Collaborator

Choose a reason for hiding this comment

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

doc singular

@@ -1,13 +1,12 @@
[tox]
envlist=py27,py35,py36,py37,py38
envlist=py27,py35,py36,py37,py38,docs
Copy link
Collaborator

Choose a reason for hiding this comment

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

docs plural

storage

[testenv:docs]
Copy link
Collaborator

Choose a reason for hiding this comment

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

docs plural

@ctb
Copy link
Contributor

ctb commented Jun 24, 2020

Issues that need to be resolved for a merge, IMO -

I might work on these in a separate PR (into this one).

@ctb
Copy link
Contributor

ctb commented Jun 27, 2020

make doc fixed in #1053; the key problem showing up with make -d was,

  Successfully remade target file `build'.
 Finished prerequisites of target file `doc'.
 Prerequisite `build' is older than target `doc'.

so basically we just need to force the cd doc && make html command. One way to do that is to add .PHONY as a prereq.

@luizirber luizirber merged commit b5c3f2e into master Jun 27, 2020
@luizirber luizirber deleted the doc_myst branch June 27, 2020 16:54
@luizirber luizirber mentioned this pull request Oct 30, 2020
10 tasks
ctb added a commit that referenced this pull request Jun 22, 2023
Fixes #2604

This PR fixes links to binder tutorials that apparently have been broken
since 2020 😱 , when the docs were switched over to myst-parser in
#1021.

To be fair, it's the kind of subtle bug that would leave users
scratching their head going "maybe it's me?" - the links simply went
back to the tutorial page... not sure why they weren't flagged by sphinx
or myst-parser as being bad!?

This PR also updates environment.yml to properly build:
* includes pip
* specifies minimum python version
* specifies minimum sourmash version

ref #2503 for why
specifying the minimum sourmash and/or python versions can be important!

And, finally, I ran and fixed all the notebooks...

Note: I confirmed that this branch launches properly in binder. The URL
is: https://mybinder.org/v2/gh/sourmash-bio/sourmash/fix/tutorial-nb

---------

Co-authored-by: ccbaumler <63077899+ccbaumler@users.noreply.github.com>
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