-
Notifications
You must be signed in to change notification settings - Fork 5
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: add extensive externally hosted docs #177
base: dev
Are you sure you want to change the base?
Conversation
.github/workflows/ci.yml
Outdated
@@ -214,3 +216,35 @@ jobs: | |||
- name: Run SRA downloads workflow | |||
run: bash tests/test_sra_download_with_conda/test.local.sh | |||
|
|||
integration-docker: |
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.
Test that pulls a zarp docker image and runs a test
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.
Cool to add a Dockerfile, test and docs for that, but I think that should go in a separate PR, if you don't mind.
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.
OK, moving it to another PR.
## Running ZARP without ZARP-cli | ||
|
||
1. Assuming that your current directory is the workflow repository's root directory, |
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.
Moved to github website
@@ -6,3 +6,4 @@ dependencies: | |||
- python>=3.10, <3.12 | |||
- snakemake<8 | |||
- graphviz | |||
- pyopenssl>=23.2.0 |
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 tests were failing with conda/conda#13619
I had to install pyopenssl
@@ -65,4 +65,6 @@ extra: | |||
nav: | |||
- Home: README.md | |||
- Installation: guides/installation.md | |||
# ADD ADDITIONAL PAGES HERE | |||
- Usage: guides/usage.md |
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.
pages for website
@@ -1,5 +1,6 @@ | |||
--- | |||
channels: | |||
- conda-forge |
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.
add missing dependency
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.
Thanks a lot @fgypas and @mkatsanto 🙏
Looks pretty good, but there are still some issues. My comments mostly focused on the most prominent issues - there are still a bunch of issues I didn't check/comment on (and some areas I didn't even read thoroughly).
It would perhaps be good to run a spell checker and/or run it through GenAI to improve the wording and consistency in some parts. And definitely use a markdown linter.
Would also be good to ask @ninsch3000 for an additional review in the next round.
Finally, given that we change dependencies (environment.yml
, samtools.yaml
), I think we will need to create another release after we merge this, then update Zenodo and the manuscript and so on.
.github/workflows/ci.yml
Outdated
@@ -214,3 +216,35 @@ jobs: | |||
- name: Run SRA downloads workflow | |||
run: bash tests/test_sra_download_with_conda/test.local.sh | |||
|
|||
integration-docker: |
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.
Cool to add a Dockerfile, test and docs for that, but I think that should go in a separate PR, if you don't mind.
README.md
Outdated
|
||
```sh | ||
zarp SRR23590181 | ||
``` |
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 previous statement doesn't make sense if you remove this...
README.md
Outdated
After successful execution - if all parameters could be either inferred or were specified by the user - `[OUTDIR]/[SAMPLES_OUT]` should contain a populated table with parameters `seqmode`, `f1_3p`, `f2_3p`, `organism`, `libtype` and `index_size` for all input samples as described in the [pipeline documentation][sample-doc]. | ||
|
||
|
||
You can also trigger ZARP without ZARP-cli. This is convenient for users who have some experience with snakemake and don't want to use a CLI to trigger their runs. Please head over to the [ZARP](https://zavolanlab.github.io/zarp/) documentation to learn how to start ZARP. |
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 having to scroll down 193 lines to find a link to the ZARP docs is a bit strange. At this point readers might be convinced they have actually been reading the ZARP docs.
I would drastically shorten this document (just leave the absolute highlights)section and put a link to the docs in a much more prominent position.
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 made it similar to zarp-cli
I updated the documentation based on the feedback. Can you please have a look again @uniqueg @ninsch3000? Thank you in advance. |
Description
Fixes #165
Type of change
Please delete options that are not relevant.
Conventional Commits guidelines
https://www.conventionalcommits.org/en/v1.0.0/
Changes to workflow inputs (sample table and/or configs)
Changes to workflow outputs
Everything else: patch
(add any other conventional commit in the beginning of the PR title)
Checklist: