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

Change .rst to .md Fix/#340 #356

Merged
merged 10 commits into from
Oct 27, 2017
Merged

Change .rst to .md Fix/#340 #356

merged 10 commits into from
Oct 27, 2017

Conversation

connor-reid-tiffany
Copy link
Contributor

Reformatted all .rst files to markdown files in sourmash/docs except for index.rst. Fix for issue #340

  • 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-io
Copy link

codecov-io commented Oct 4, 2017

Codecov Report

Merging #356 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #356   +/-   ##
=======================================
  Coverage   87.15%   87.15%           
=======================================
  Files          14       14           
  Lines        2087     2087           
  Branches       36       36           
=======================================
  Hits         1819     1819           
  Misses        267      267           
  Partials        1        1

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 2c2552a...1a079bf. Read the comment docs.

Copy link
Contributor

@standage standage left a comment

Choose a reason for hiding this comment

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

Thanks for this pull request, Connor. Almost there! It looks like this task was a little bit more involved than anticipated!

Minor changes:

  • I've highlighted a few places in where you've retained the code block indentation. This can and probably should be removed.
  • More generally, some of the spacing between sections is inconsistent. Sometime there are two or three lines between sections, and sometimes there are none (this makes it hard to scan through the Markdown and easily identify breaks between sections). We don't have any hard and fast rules about spacing, as long as everything renders ok. But it would be helpful to be consistent! :)

Major change:

  • Several places in the command-line.md document it appears that you've copied and pasted some automatically generated text instead of using what are called "directives" in the Sphinx documentation system. We'll want to fix this before merging.

Let us know if anything is unclear. Thanks!


* [`sourmash compare`](### `sourmash compare`)

* [`sourmash plot`](### `sourmash plot`)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this block isn't formatted correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you probably want the .. contents:: directive here. You've manually pasted in some links that should be dynamically generated by the documentation build system

Copy link
Contributor

Choose a reason for hiding this comment

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

I may have been mistaken here too. If the .. contents:: directive doesn't work in Markdown, we'll have to figure out how to add anchors to the different sections—or simply just delete the contents block!

```
curl -L -O ftp://ftp.ncbi.nlm.nih.gov/genomes/refseq/bacteria/Escherichia_coli/reference/GCF_000005845.2_ASM584v2/GCF_000005845.2_ASM584v2_genomic.fna.gz
curl -L -O ftp://ftp.ncbi.nlm.nih.gov/genomes/refseq/bacteria/Salmonella_enterica/reference/GCF_000006945.1_ASM694v1/GCF_000006945.1_ASM694v1_genomic.fna.gz
curl -L -O ftp://ftp.ncbi.nlm.nih.gov/genomes/refseq/bacteria/Sphingobacteriaceae_bacterium_DW12/latest_assembly_versions/GCF_000783305.1_ASM78330v1/GCF_000783305.1_ASM78330v1_genomic.fna.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge deal, but since other changes are required, should remove the leading spaces here.

Copy link
Contributor

Choose a reason for hiding this comment

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

And in the other code blocks in this file.

gzip or bzip2. The output will be one or more JSON signature files
that can be used with `sourmash compare`.

Usage:
Copy link
Contributor

Choose a reason for hiding this comment

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

Also a copy/paste mistake. Instead of typing this out fully, this should be auto-generated with the .. Usage:: directive. That way changes to the code are automatically reflected in the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I was mistaken. This is not a directive.

doc/developer.md Outdated
```
python -m virtualenv dev
. dev/bin/activate
make clean test
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop all the leading spaces in each code block. In RST the indentation indicates that this is a code block. Since Markdown uses tick marks, the leading spaces aren't needed.

… file with all the info from the previous index.rst file. fixed hyperlinks in all the edited files.
@standage
Copy link
Contributor

standage commented Oct 26, 2017

It's looking much better now indeed. But having nothing but a table of contents on the index page is a bit jarring, and having a separate welcome page is a bit awkward.

You've had to go down the rabbit hole a bit with this PR, and try a lot of things to get it all to work. It might be helpful at this point to come back out of the rabbit hole a bit and re-assess. If you revert doc/index.rst to what it was before you started, I think everything should work as intended. Then you can drop this new welcome page you created, revert the changes to doc/_templates/navigation.html, and then focus on whitespace cleanup, at which point I think this PR will be ready to go!

Oh, there's also a merge conflict as well, but this should be simple to address.

@luizirber
Copy link
Member

@recursive-deletion If you run git diff master.. -- index.rst you can see the differences to the original file

@luizirber
Copy link
Member

Looking good! A small nitpick is that some files were added/removed without git keeping track of it (doc/command-line.md and doc/command-line.rst, for example). Wanna tackle that next? =]

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.

4 participants