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

Declutter README #2232

Merged
merged 18 commits into from
Apr 25, 2023
Merged

Declutter README #2232

merged 18 commits into from
Apr 25, 2023

Conversation

grst
Copy link
Member

@grst grst commented Apr 3, 2023

Simplifies README as discussed in #2186.

Close #2186.

PR checklist

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

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as off-topic.

Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

Overall looks good, a few comments

nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
@grst grst changed the base branch from master to dev April 3, 2023 12:52
@grst grst force-pushed the update-readme-template branch from 4ab1416 to 8c714e7 Compare April 3, 2023 12:54
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #2232 (6437099) into dev (fa05c3c) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 6437099 differs from pull request most recent head 53177bc. Consider uploading reports for the commit 53177bc to get more accurate results

@@            Coverage Diff             @@
##              dev    #2232      +/-   ##
==========================================
+ Coverage   73.04%   73.07%   +0.02%     
==========================================
  Files          77       77              
  Lines        8344     8333      -11     
==========================================
- Hits         6095     6089       -6     
+ Misses       2249     2244       -5     
Impacted Files Coverage Δ
nf_core/lint/readme.py 81.81% <ø> (+6.06%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@grst grst requested a review from jfy133 April 5, 2023 07:35
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

LGTM! Only added a couple of small suggestions. Thank you 🙂

nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
@jfy133
Copy link
Member

jfy133 commented Apr 20, 2023

Only last comment is I wonder if we should have more emphasis on where to find the documentation, I wonder if it needs it's own section and a sentence saying 'check the website' sortof thing (rather than just at the bottom of the usage section)

@grst
Copy link
Member Author

grst commented Apr 20, 2023

we should have more emphasis on where to find the documentation

it's also at the bottom of the output section. Personally, I think it's ok as it is, but I don't mind another link (or maybe badge?).

@ewels
Copy link
Member

ewels commented Apr 20, 2023

I find reading the template diff awkward here, so I just generated a before- and after- from nf-core create. Attaching in case it's useful for others:

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Massive improvement, thanks @grst!

Made a few suggestions, let me know what you think.

nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
@grst grst requested a review from ewels April 23, 2023 17:17
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Couple of very minor grammar changes, otherwise LGTM to merge.

Massive improvement - thank you @grst!

nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
nf_core/pipeline-template/README.md Outdated Show resolved Hide resolved
@ewels ewels merged commit 93defc7 into dev Apr 25, 2023
@ewels ewels deleted the update-readme-template branch April 25, 2023 11:21
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.

Improve pipeline overview documentation
4 participants