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

Major overhaul of docs #219

Merged
merged 13 commits into from
Nov 29, 2018
Merged

Major overhaul of docs #219

merged 13 commits into from
Nov 29, 2018

Conversation

drpatelh
Copy link
Member

@drpatelh drpatelh commented Nov 28, 2018

  • This comment contains a description of changes (with reason)
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated
  • CHANGELOG.md is updated
  • README.md is updated

  • Removed docs for profiles standard and none and changed description where applicable
  • Fixed a couple of broken links
  • Added new parameters --igenomesIgnore and --custom_config_version to usage.md
  • Rearranged sections in README.md and docs/README.md to be consistent
  • Added TODO statement in troubleshooting.md
  • Added some description for providing custom config profiles
  • Addressed a few other issues I noticed whilst writing nf-core/atacseq

@codecov-io
Copy link

codecov-io commented Nov 28, 2018

Codecov Report

Merging #219 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #219   +/-   ##
=======================================
  Coverage   90.76%   90.76%           
=======================================
  Files           8        8           
  Lines         931      931           
=======================================
  Hits          845      845           
  Misses         86       86

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 1702fbd...4857789. Read the comment docs.

Copy link
Member

@sven1103 sven1103 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @drpatelh for this! Only minor things from my side to correct.

@@ -242,7 +241,7 @@ Use to set a top-limit for the default CPU requirement for each process.
Should be a string in the format integer-unit. eg. `--max_cpus 1`

### `--plaintext_email`
Set to receive plain-text e-mails instead of HTML formatted.
Set to receive plain-text e-mails instead of HTML formatted.
Copy link
Member

Choose a reason for hiding this comment

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

Added whitespace? I think this can be reverted

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now in latest commit.


### `--multiqc_config`
### `--multiqc_config`
Copy link
Member

Choose a reason for hiding this comment

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

Don't see a change here, probably different encoding?

Copy link
Member Author

Choose a reason for hiding this comment

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

Im a bit puzzled too.

The --multiqc_config section isnt rendered properly in the current usage.md:
https://github.com/nf-core/tools/blob/dev/nf_core/pipeline-template/%7B%7Bcookiecutter.name_noslash%7D%7D/docs/usage.md#--plaintext_email

Im not sure whether the whitespace in the comment above sorts it out but its correctly rendered in this version:
https://github.com/drpatelh/tools/blob/dev/nf_core/pipeline-template/%7B%7Bcookiecutter.name_noslash%7D%7D/docs/usage.md#--multiqc_config

I got this working for the docs in nf-core/atacseq so I copied it across and looked at the diff. Weird but it seems to work. I was staring at it for ages!

Copy link
Member

Choose a reason for hiding this comment

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

It's a non breaking space that can be achieved by holding shift and pressing space*. It looks identical in text editors and GitHub, but breaks GitHub markdown processing. Delete and press space again carefully and everything will work as expected.

* Learnt from a frustratingly large number of times where I've done this myself

Copy link
Member

Choose a reason for hiding this comment

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

holy, @ewels wtf. Yeah next guess would have been to check character encoding ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Seeing as what i have done is doing the trick is it ok to leave it as it is? Not sure if it's likely to break something else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now in latest commit. I think...

sven1103 and others added 2 commits November 28, 2018 19:00
…onfiguration/adding_your_own.md

Co-Authored-By: drpatelh <drpatelh@users.noreply.github.com>
Wherever process-specific requirements are set in the pipeline, the default value can be changed by creating a custom config file. See the files in [`conf`](../conf) for examples.
Wherever process-specific requirements are set in the pipeline, the default value can be changed by creating a custom config file. See the files hosted at [`nf-core/configs`](https://github.com/nf-core/configs/tree/master/conf) for examples.

If you are likely to be running `nf-core` pipelines regularly it may be a good idea to request that your custom config file is uploaded to the `nf-core/configs` git repository. Before you do this please can you test that the config file works with your pipeline of choice using the `-c` parameter (see definition below). You can request your custom config file to be added in the `pipelines` chat on the Slack messenger for `nf-core` (may need to be invited first!) or on the `nf-core/Lobby` group on Gitter.
Copy link
Member

Choose a reason for hiding this comment

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

No need to mention Gitter. Perhaps instead say that people can create a pull request with their config and should join slack to become a member of nf-core and ask for help..

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can drop gitter as we will most likely move everyone over to slack anyways...

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't you have to be invited to join the Slack group? Or is that likely to change?

I was anticipating the rather tedious scenario where you ask for an invite via Gitter to join Slack...

Copy link
Member

Choose a reason for hiding this comment

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

No not anymore: https://nf-core-invite.herokuapp.com/

I also put this up to the Gitter channel (as top most message in the channel description) and we should add it to the main homepage...

Copy link
Member

Choose a reason for hiding this comment

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

Might add it to the homepage today with a logo, I guess its time

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added details in latest commit.

@@ -229,9 +225,12 @@ Specify the path to a specific config file (this is a core NextFlow command).

Note - you can use this to override pipeline defaults.

### `--custom_config_version`
Provide git commit id for custom Institutional configs hosted at `nf-core/configs`. This was implemented for reproducibility purposes. Default is set to `master`.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe give an example of providing an exact commit ID..

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in latest commit.

Copy link
Member

@apeltzer apeltzer left a comment

Choose a reason for hiding this comment

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

Looking good now :-)

@@ -228,6 +230,11 @@ Note - you can use this to override pipeline defaults.
### `--custom_config_version`
Provide git commit id for custom Institutional configs hosted at `nf-core/configs`. This was implemented for reproducibility purposes. Default is set to `master`.

```bash
## Download and use config file with following git commid id
--custom_config_version d52db660777c4bf36546ddb188ec530c3ada1b96
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@sven1103 sven1103 merged commit 3390899 into nf-core:dev Nov 29, 2018
@sven1103
Copy link
Member

thanks a lot @drpatelh for this contribution 🎉

@drpatelh
Copy link
Member Author

@sven1103 Youre very welcome!

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.

5 participants