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

Expand contributors guide in a new document #1337

Closed
2 of 11 tasks
jneira opened this issue Feb 9, 2021 · 7 comments
Closed
2 of 11 tasks

Expand contributors guide in a new document #1337

jneira opened this issue Feb 9, 2021 · 7 comments
Labels
type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. type: enhancement New feature or request

Comments

@jneira
Copy link
Member

jneira commented Feb 9, 2021

The guide should include imo

  • A description of the policy followed by maintainers about code reviews and merges of pull requests
    • With a description of the mergify + ci checks workflow we are using
    • Noting that you can open draft pr's to signal your work
    • Noting that pr's over an existing issue/feature request will be more welcomed (but it would be no a required condition, as the pr itself could be interesting)
  • Technical notes about how to open am appropiate pull request:
  • A list of maintainers that can be pinged to ask for help, code review, etc
  • We should create a pull request template with some basic information and a link to the guide

The goal is make clear potential contributors what we expect and leverage the most of maintainers and contributors work

We have currently two places with documentation related:

@jneira
Copy link
Member Author

jneira commented Feb 9, 2021

For example, i really like the philosophy and redaction of dhall-lang's contributor guide: https://github.com/dhall-lang/dhall-lang/blob/master/.github/CONTRIBUTING.md

@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Dec 21, 2021

I believe #2517 is partially related.

@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Dec 21, 2021

Noting that you can open draft pr's to signal your work

I believe that PR should be marked green only if it is at MVP stage to be considered by maintainers to merge it. Some PRs keep being green while being abandoned for years on end - sending the mixed signal to maintainers. For example - if central maintainer said that rebase is too hard & needs a topic-knowing rebase - it means that PR is currently not mergable & so a draft that awaits an interested narrow-skilled person (often author) to carefully do a rebase to turn PR into mergable.

  1. (WIP) prefix in names - is just a euphemism for the draft status.

  2. When PRs would become ready - contributor would turn the PR green themselves, even if just to get attention, if the contributor is too shy to notify directly. Contributor can be 1) shy, 2) can do not know who from maintainers to call, or 3) do not call specific person for review (increasingly true polite behavior - the more team grows - as available people more and more can cross-cover each other), etc. So in those case contributor frequently would not send a direct review requests.

So contributor implicitly is driven to mark PR green - seen as ready - so maintainers would be attracted to devote attention to it. & maintainers would see color code & the status change & would look at the progress.

After cleaning of the PR list (went over the PRs & triaged the status & politely thanked & asked about the continuation & noted that PR is WIP & set its status as a draft) - we now see mostly which PRs are in the process. I left only 2 old PRs green, which seemed very close to be resolved/merged or which are important to remember about.

  1. If it is a draft - people do not just into the thread, draft status gives both sides a piece of mind, as a contributor has draft status protection & has that strong context & argument keep community bikeshedding, nitpicking, or future feature requests at bay, which makes PRs easier to read/review the status of & to merge.

So now when old PRs would become green - we know contributor considered it ready.


Of course, I myself fall into the sin of making PRs green in advance sometimes, dreaming of an early fast happy path & to attract review in advance, but time & time again I find that keeping WIP PR as a draft - helps to focus on doing useful work quite a bit.

@jneira
Copy link
Member Author

jneira commented Dec 21, 2021

Agree in use the draft pr as a strong mark for both authors and maintainers. But i was a little bit reluctant to change its state (although i did it several times), cause each author could think a different thing about what means "draft".
So i used several labels (failing tests, needs rebase, etc) to mark prs and help in their managing, without crossing the draft line.
It also has the advantage you know the state with a little bit of more detail (github already shows other things as revisions state and ci)

Nevertheless i think the key think is document what is the meaning we aim to give to the draft state to help authors. I think your comment could be the starting point. Extending it with instructions about what to do if you want earlier feedback, post questions, etc. Sometimes draft pr's are the ones needing attention.

@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Dec 21, 2021

Sometimes draft pr's are the ones needing attention.

Agreed. That is why I left several as green, since they looked too important & as needed to remember about/bounce people on then until it resolves.

As not only maintainers are attracted to green PRs (besides attracting the eye, people also like to look "what is going on in the pipeline" & what are new features are ready to be merged now), so green status also attacks contributor attention & maybe someone quite knowledgeable would arrive & give superb suggestion, review or would do a knowledgable rebase to help ship the feature. But to focus the attention on drive-by experts - only some of PRs need to be green.

cause each author could think a different thing about what means "draft"

Yes, this is why I almost always politely bounce a message to the authors mentioning the status of the thread (notifying why it seems WIP currently). Rebase churn is real, I rebased several PRs yesterday, but there are PRs that only mostly the author can rebase. & I also may not put labels somewhere (still learning).

@jneira
Copy link
Member Author

jneira commented Dec 21, 2021

sure, and thanks for your work triaging issues and prs, it is very very welcomed

@fendor fendor added type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. and removed old_type: documentation labels Jul 13, 2022
@michaelpj
Copy link
Collaborator

We have a contributing guide. It could have more stuff, but this is pretty old

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants