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

add draft to guidelines from slack discussion #2780

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

lpantano
Copy link
Contributor

@lpantano lpantano commented Oct 9, 2024

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

Copy link

netlify bot commented Oct 9, 2024

Deploy Preview for nf-core-main-site ready!

Name Link
🔨 Latest commit 0a46cba
🔍 Latest deploy log https://app.netlify.com/sites/nf-core-main-site/deploys/6722a6ddfddb0000084c313b
😎 Deploy Preview https://deploy-preview-2780--nf-core-main-site.netlify.app/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 9, 2024

Deploy Preview for nf-core-docs ready!

Name Link
🔨 Latest commit 0a46cba
🔍 Latest deploy log https://app.netlify.com/sites/nf-core-docs/deploys/6722a6dd2329f6000824da2d
😎 Deploy Preview https://deploy-preview-2780--nf-core-docs.netlify.app/docs/tutorials/contributing_to_nf-core/contributing_to_pipelines
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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.

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..

- 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).
Copy link
Member

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

Copy link
Contributor Author

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

@ewels
Copy link
Member

ewels commented Oct 9, 2024

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.

@ewels
Copy link
Member

ewels commented Oct 9, 2024

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.

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.

Couple of prhasing tweaks but overall LGTM after the indentation from Phil (agree reads a bit weird as they are explanation)

@lpantano
Copy link
Contributor Author

lpantano commented Oct 9, 2024

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.
Agree to have it outside tutorial, are you thinking Guidelines/Pipelines?

@lpantano
Copy link
Contributor Author

lpantano commented Oct 9, 2024

or just under Guidelines?

@lpantano
Copy link
Contributor Author

@ewels and @jfy133 , I think this is almost there. Just need to figure out the final place for this. Is inside Guidelines a good place?

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.

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.

lpantano and others added 5 commits October 29, 2024 09:13
…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]>
@jfy133
Copy link
Member

jfy133 commented Oct 30, 2024

@nf-core-bot fix linting

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.

4 participants