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

Improve contributing guide #1513

Open
10 tasks
KevinMind opened this issue Nov 3, 2022 · 5 comments · May be fixed by #2869
Open
10 tasks

Improve contributing guide #1513

KevinMind opened this issue Nov 3, 2022 · 5 comments · May be fixed by #2869
Assignees
Labels
c: docs Improvements or additions to documentation p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@KevinMind
Copy link
Contributor

KevinMind commented Nov 3, 2022

Following up on #1413 (comment) I'd like to improve the contributing guide. Keeping a short list of changes, with a few open questions needing resolution before opening the PR.

  • update contributing guide with fork workflow, using screenshots I snapped.
  • update contributing guide with more details about PRs
    • should PRs specify tasks that they close/relate to. how so?
    • anything to put in body?
    • commits after PR is opened, force or not force?
    • how to resolve comments before merging?
    • can you deploy/test code before merging?
    • before merging, should we squash rebase?
    • actual merge etiquette. Can someone merge after approvals? Is there a process or steps to follow?
  • Link to release process.
@ST-DDT ST-DDT added c: docs Improvements or additions to documentation s: needs decision Needs team/maintainer decision labels Nov 3, 2022
@KevinMind
Copy link
Contributor Author

Question: Is this normal? When re-requesting review it only allows to re-request from one reviewer. If normal, we should document that behaviour, if not, maybe fix it?

Screen.Recording.2022-11-06.at.11.35.30.mov

@import-brain
Copy link
Member

Question: Is this normal? When re-requesting review it only allows to re-request from one reviewer. If normal, we should document that behaviour, if not, maybe fix it?

Screen.Recording.2022-11-06.at.11.35.30.mov

I don't believe this behavior is intended, try refreshing the page to see if it registers.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 16, 2023

  • update contributing guide with fork workflow, using screenshots I snapped.

We will decide later what to do here.

  • should PRs specify tasks that they close/relate to. how so?
  • anything to put in body?

We should update the PR template with

Fixes #1513

- #1513

...
  • commits after PR is opened, force or not force?

Push commits until the PR is ready, nothing to document here.

  • how to resolve comments before merging?

If the issue is explicitly resolved (e.g. typo is fixed) mark the comment as resolved.
If not, don't. We will re-review the code for new/resolved issues in the changed code anyway.

  • can you deploy/test code before merging?

Document how to use "overwrites".

  • before merging, should we squash rebase?

No, do not squash as this complicates the review process. We will squash the PR for you. This doesn't have to be documented as most open source projects do this like this.

  • actual merge etiquette. Can someone merge after approvals? Is there a process or steps to follow?

Only the team can merge, so nothing external contributors can do. We have rules when to merge PRs (wait times for more reviews), but these are irrelevant for others and thus don't need to be documented in the contribution guidelines.

  • Link to release process.

External contributors cannot release (under our name) and thus this doesn't have to be documented in the contribution guidelines.
We will document the release steps, when we update/automate the process in a separate document.

@matthewmayer
Copy link
Contributor

Only the team can merge, so nothing external contributors can do. We have rules when to merge PRs (wait times for more reviews), but these are irrelevant for others and thus don't need to be documented in the contribution guidelines.

Even if there's nothing a contributor can do, it's nice to give people some kind of clarification like "don't worry if you open a PR and after some initial comments, nothing seems to happen for a few weeks, it will take time for the maintainers to review the code, decide to accept it, and then merge."

@ST-DDT
Copy link
Member

ST-DDT commented Oct 24, 2023

Team Decision

  • update contributing guide with fork workflow, using screenshots I snapped.
  • update contributing guide with more details about PRs
    • commits after PR is opened, force or not force?
    • how to resolve comments before merging?
    • can you deploy/test code before merging?
    • before merging, should we squash rebase?
    • actual merge etiquette. Can someone merge after approvals? Is there a process or steps to follow?

We will review existing contribution.md regarding these topics.
Additionally, we will create a contribution guide similar to eslint's on our website which will include multiple sections on this topic.
This will include the results of the review of the contribution.md
Our contribution.md will include a link to our website

  • should PRs specify tasks that they close/relate to. how so?
  • anything to put in body?
  • Extend PR template with hints like
Fixes #<issue>

- #<issue>

Does xyz

This will also be included in the future "how to contribute/PR" section.

  • Link to release process.

We will add a section similar to eslint's.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent and removed s: needs decision Needs team/maintainer decision labels Oct 24, 2023
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: docs Improvements or additions to documentation p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

5 participants