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 11 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 cf64f1b
🔍 Latest deploy log https://app.netlify.com/sites/nf-core-main-site/deploys/6706ede2489714000859bcae
😎 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 cf64f1b
🔍 Latest deploy log https://app.netlify.com/sites/nf-core-docs/deploys/6706ede247a34d0008e10eeb
😎 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 and others added 9 commits October 9, 2024 16:43
…contributing_to_pipelines.md

Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
@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?

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.

3 participants