-
Notifications
You must be signed in to change notification settings - Fork 192
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
add draft to guidelines from slack discussion #2780
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for nf-core-main-site ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for nf-core-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Needs a little polishing yet I think, left a few comments. Nothing major though.
What do folks think about having this as a new dedicated Guidelines page and linking to it from here?
I had envisioned it more as a set of rules / standards that we follow rather than instructions in a tutorial tbh..
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
- Authors/Reviewers can request help for review at nf-core Slack `#request-review` channel. | ||
- Anyone can merge after one positive review and with no obvious open questions and, merge ASAP so as not to leave approved PRs hanging. | ||
- Exception in merges to master for releases (two reviews). | ||
- Exception in pseudo PR review for first release (needs review from core or maintainer team). |
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.
Link to docs for first release
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.
sorry, I cannot find exactly this? I found pipeline release but not first release
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
One to add: "request changes" reviews can be dismissed by the PR author if considered out of date and resolved. Link to the GitHub docs on this to explain how to do it. |
Also maybe some mention that it's fine / encouraged for other people to push to your PR. For example fixing linting (can mention bot commands) but also to build on or fix code if appropriate. |
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.
Couple of prhasing tweaks but overall LGTM after the indentation from Phil (agree reads a bit weird as they are explanation)
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
…contributing_to_pipelines.md Co-authored-by: James A. Fellows Yates <[email protected]>
Co-authored-by: James A. Fellows Yates <[email protected]>
Co-authored-by: James A. Fellows Yates <[email protected]>
Co-authored-by: James A. Fellows Yates <[email protected]>
… add-PRreview-guides
thanks! I made a bunch of changes following the comments, much better now, not sure if I am 100% in the indents of all the points, let me know. |
or just under Guidelines? |
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.
Ugh sorry @lpantano ! The first re-review request got buried in my notifications 🙈 Feel free to bother me on slack next time :)
Re the location: Reading through it I think a guidelines page would make sense - you use a lot of 'SHOULD' language which would fit, but if we do move there I feel we need to tweak the language to follow e.g. the modules guidelines (with a strict definition of SHOULD/MUST/MUST NOT) etc.
Each major point in this list should be become it's own header which we can then number using that astro plugin @mashehu has added to the top of e.g. the modules guidelines, and we can 'cite' them.
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
sites/docs/src/content/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines.md
Outdated
Show resolved
Hide resolved
…contributing_to_pipelines.md Co-authored-by: James A. Fellows Yates <[email protected]>
…contributing_to_pipelines.md Co-authored-by: James A. Fellows Yates <[email protected]>
…contributing_to_pipelines.md Co-authored-by: James A. Fellows Yates <[email protected]>
…contributing_to_pipelines.md Co-authored-by: James A. Fellows Yates <[email protected]>
@nf-core-bot fix linting |
Following slack discussion we agree on adding some text on PR review to make it more transparent to new people or as a reminder.
@netlify /docs/tutorials/contributing_to_nf-core/contributing_to_pipelines