-
Notifications
You must be signed in to change notification settings - Fork 55
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
(Partial) Fix #240 #245
(Partial) Fix #240 #245
Conversation
Removes outdated references to v0.1 Adds additional information for installation of v0.2 using conda, pip, and from downloaded source code. Hopefully this is clearer. Adds a more strident notice about the relationship between 0.2 and 0.3 (0.2 maintenance-only; default repo is 0.3; etc.) Updated copyright years
Removes outdated references to v0.1 Adds additional information for installation of v0.2 using conda, pip, and from downloaded source code. Hopefully this is clearer. Adds a more strident notice about the relationship between 0.2 and 0.3 (0.2 maintenance-only; default repo is 0.3; etc.) Updated copyright years
Updates installation instructions with an emphasis on clarifying that the default repository branch is the development version (v0.3) and that conda/pip will install v0.2.
Codecov Report
@@ Coverage Diff @@
## master #245 +/- ##
=======================================
Coverage 75.82% 75.82%
=======================================
Files 52 52
Lines 3239 3239
=======================================
Hits 2456 2456
Misses 783 783 |
Not looked at the detailed changes, but the pull request summary sounds sensible. |
README.md
Outdated
Then install `pyani`: | ||
### Third-party tools | ||
|
||
Three alignment packages are required, to use all of `pyani`'s methods: `mummer`, `BLAST+`, and legacy `BLAST`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth stating that if they only want to use some of pyani
's methods, they could only install the ones they want?
(Assuming, of course, that doesn't break something.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for clarity, yes. I wouldn't expect it to break anything.
README.md
Outdated
|
||
If you are using an older version of `pyani` (v0.2.x), then please note that the command-line API has changed, and documentation for this version can be found at the following page: | ||
If you are using the stable version of `pyani` (v0.2.x), then please note that the command-line API has changed, and documentation for this version can be found at the following page: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This references a change in the command-line API for v0.2.x? The wording is a bit ambiguous. It could be taken to mean an intra-version change, or to refer to the difference between version 2 and version 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the stable
is clearer only until v0.3 is the stable version, but an older
is unnecessary and could be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that addresses my question, which is caused by the second phrase. I only know the CLI for v0.2 and v0.3 as they are now, so I am unclear whether the referenced change is:
- the one between v0.2 and v0.3, and you're warning them that the documentation they see is not applicable to users of the stable version; or
- if you mean a change from a previous version of v0.2 and the current, stable version, and you're warning people they may need to update their scripts if they update
pyani
to the current v0.2.
I think, given the wording, the second option seems more likely, but I don't know if there were any changes to the CLI in v0.2.
``` | ||
|
||
## Docker images | ||
|
||
`pyani`'s scripts are also provided as [Docker](https://www.docker.com/) images, that can be run locally as containers. To use these images, first install Docker, then to run the corresponding scripts issue either: | ||
`pyani` v0.2 scripts are also provided as [Docker](https://www.docker.com/) images, that can be run locally as containers. To use these images, first install Docker. Then, to run the corresponding scripts, issue either: | ||
|
||
```bash | ||
docker run -v ${PWD}:/host_dir leightonpritchard/average_nucleotide_identity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@widdowquinn
I've never used Docker. Should this really have a directory named for you in the example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That's the name of the account that hosts the image on DockerHub (see, e.g. here). The command pulls and runs leightonpritchard/average_nucleotide_identity
from Dockerhub.
I'd be open to having a neutral DockerHub account to host these tools. Possibly this would be best under the collective genomeRxiv name.
- **BLAST+** (for `anib`) [ftp://ftp.ncbi.nlm.nih.gov/blast/executables/blast+/LATEST/](ftp://ftp.ncbi.nlm.nih.gov/blast/executables/blast+/LATEST/) | ||
- **legacy BLAST** (for `aniblastall`) [ftp://ftp.ncbi.nlm.nih.gov/blast/executables/release/LATEST/](ftp://ftp.ncbi.nlm.nih.gov/blast/executables/release/LATEST/) | ||
- **MUMmer** (for `anim`) [http://mummer.sourceforge.net/](ftp://ftp.ncbi.nlm.nih.gov/blast/executables/release/LATEST/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the preview, it looks like none of these links are actually being created. I don't know why this would be as the link for the bioconda site earlier works and they seem to be formatted correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The in situ GitHub Markdown rendering is a bit of a mystery to me. It's not always consistent. VS Code renders the links correctly, and I would expect Jekyll/GitHub pages to render correctly, also.
If the links are visible, they can be copy/pasted, so it's not a fatal issue. Worth keeping an eye on, though.
**THIS README AND THE DOCUMENTATION AT `READTHEDOCS` REFERS TO A DEVELOPMENT VERSION OF `PYANI` (v0.3+). IT HAS A DIFFERENT COMMAND-LINE INTERFACE THAN THE STABLE `PYANI` VERSION (v0.2.x).** | ||
|
||
**THE STABLE VERSION OF `PYANI` (v0.2) DOES NOT HAVE THE `pyani` COMMAND** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of putting things like this inside of a callout box? I have not been able to find a quick solution for doing this in GitHub Markdown, but even if there is no way to do it in pure markdown, html/css will work. There are a few places where I think it would make it easier to differentiate notate bene from the rest of the text—perhaps more-so than bold all-caps does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the appropriate circumstances, a callout box would be perfect.
As you note, it relies on the renderer, and its willingness to parse and present HTML/CSS. Here I'm targeting GitHub's default rendering, and the default Jekyll/GitHub Pages setup. They're rather limited in scope - as is Markdown.
You'll see on the ReadTheDocs pages I make liberal use of callouts, etc. when it's available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've always found html and css to interact well with markdown, even without something like Jekyll; I'll see if I can get something non-complex working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the other consideration is that README.md
takes the place of README.txt
. The document needs to be legible when it's not viewed through a renderer - e.g. going through the doc with less
. On balance I think it's best to keep the README.md
formatting relatively light, to avoid a level of HTML/CSS that obscures reading of the text.
In practice, this means things like avoiding inline CSS at the top of the page and, since GitHub doesn't let you use external CSS for README
s, I've stuck to the inbuilt Markdown formatting.
- **BLAST+** (for `anib`) [ftp://ftp.ncbi.nlm.nih.gov/blast/executables/blast+/LATEST/](ftp://ftp.ncbi.nlm.nih.gov/blast/executables/blast+/LATEST/) | ||
- **legacy BLAST** (for `aniblastall`) [ftp://ftp.ncbi.nlm.nih.gov/blast/executables/release/LATEST/](ftp://ftp.ncbi.nlm.nih.gov/blast/executables/release/LATEST/) | ||
- **MUMmer** (for `anim`) [http://mummer.sourceforge.net/](ftp://ftp.ncbi.nlm.nih.gov/blast/executables/release/LATEST/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These links also don't look like they're working in the preview.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I think that's another GitHub Markdown rendering choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is generally an improvement; it certainly makes it harder to mistake which version you're getting, and seems more explicit about how to set everything up.
I marked a few things for consideration, though whether this is the right place to handle them is debatable.
Just some random typos I found.
Fixing grammar/spelling
Co-authored-by: Bailey Harrington <baileythegreen@gmail.com>
Many thanks for the review - it brought up quite a few issues, and we're as well discussing them here, as anywhere ;) I've made a couple of changes to address some of the points you raised, and will commit, now. |
These documentation changes are intended to clarify that the default GitHub branch is the development version (v0.3), and that
conda
/pip
will install the stable version (0.2).While writing it, I thought that the better choice would be to restore
version_0_2
as thedefault
branch onGitHub
and write its documentation accordingly.If we agree on that, I propose we accept the documentation changes here as an interim measure, then start a new issue to change over the default branch and update the documentation to reflect that.