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.

Sign up for GitHub

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

Jump to bottom

[MISC] Adding formal decision-making rules #104

Merged
merged 10 commits into from
Feb 9, 2019
Merged

[MISC] Adding formal decision-making rules #104

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2ba7c0e
adding decision making rules
chrisgorgo Dec 9, 2018
c5eae0f
Update DECISION-MAKING.md
satra Dec 9, 2018
6a58d12
Update DECISION-MAKING.md
sappelhoff Dec 9, 2018
d59e839
politeness
chrisgorgo Dec 15, 2018
8b1dfcd
formatting
chrisgorgo Dec 15, 2018
8d7a625
Clarify the role of CODEOWNERS
chrisgorgo Dec 26, 2018
791e62e
Require new contributors to be added to CODEOWNERS
chrisgorgo Dec 28, 2018
d49e592
@chrisfilo wants to monitor new releases
chrisgorgo Jan 9, 2019
9e6571c
Update CONTRIBUTING.md
KirstieJane Jan 14, 2019
100e45b
Increase the number of required reviews to two
chrisgorgo Jan 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CODEOWNERS
View file
Open in desktop
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/src/01-common-principles.md @chrisfilo
/src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md @chrisfilo
4 changes: 4 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,10 @@ GitHub has a [nice introduction](https://help.github.com/articles/github-flow/)

<br>

## How the decision to merge a pull request is made?

The decision-making rules are outlined [here](DECISION-MAKING.md).
chrisgorgo marked this conversation as resolved.
Show resolved Hide resolved

## Recognizing contributions

BIDS follows the [all-contributors](https://github.com/kentcdodds/all-contributors) specification, so we welcome and recognize all contributions from documentation to testing to code development. You can see a list of current contributors in the [BIDS specification](https://github.com/bids-standard/bids-specification/blob/master/src/99-appendices/01-contributors.md).
Expand Down
79 changes: 79 additions & 0 deletions DECISION-MAKING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# Decision-making rules
## Introduction
The Brain Imaging Data Structure (BIDS) community set out the following
decision-making rules with the intention to:

- Strive for consensus.
- Promote open discussions.
- Minimize the administrative burden.
- Provide a path for when consensus cannot be made.
- Grow the community.
- Maximize the [bus factor](https://en.wikipedia.org/wiki/Bus_factor) of the
project.

The rules outlined below are inspired by the [lazy consensus system used in the Apache Foundation](https://www.apache.org/foundation/voting.html)
and heavily depends on [GitHub Pull Request Review system](https://help.github.com/articles/about-pull-requests/).

## Definitions
**Repository** - [https://github.com/bids-standard/bids-specification](https://github.com/bids-standard/bids-specification)
**Contributor** - a person listed in the Appendix I: Contributors. The
community decides on the content of this file using the same process as any
other change to the Repository (see below) allowing the meaning of "Contributor"
to evolve independently of the Decision-making rules.

## Rules
1. Every modification of the specification (including a correction of a typo,
adding a new Contributor, an extension adding support for a new data type, or
others) or proposal to release a new version needs to be done via a Pull
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little off-topic, but are there any guidelines for determining when there should be a new release? (e.g. a new release is drafted monthly, after a certain number of commits, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no such guidelines currently.

Request (PR) to the Repository.
1. Anyone can open a PR (this action is not limited to Contributors).
1. A PR is eligible to be merged if and only if these conditions are met:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an orthogonal comment to the ongoing discussion, but something like this could be incorporated in the pull request template with checkboxes to tick before merging

1. The last commit is at least 5 working days old to allow the community to
evaluate it.
1. The PR features at least one [Review that Approves](https://help.github.com/articles/about-pull-request-reviews/#about-pull-request-reviews)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the community feedback so far, it sounds like we have a reasonable number of contributors who would be willing to review PRs. Do you think we could bump the number of required reviews up to 2, as @jbpoline suggests, or even 3 ?

The reasoning, for me, is that having a higher bar to merging might be one way to ensure relative stability without moving towards maintainers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely try this and see how it goes, however, see actual engagement in reviewing PRs on this repo (even when people were explicitly invited to review) I remain skeptical we will be able to get two reviews per PR often. Please mind that increasing the bar for a review (and reviews need to be redone each time PR changes) can lead to poor contributor experience. Imagine submitting a PR and waiting for a few months to reach the number of required reviews.

We can experiment and adjust this number as we learn more about how different scenarios work. I am happy with 1 or 2, but 3 would be an overkill seeing how little people review so far.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've certainly experienced this and know it's quite frustrating ! In those experiences, I think I would have felt better if I knew what step in the review process was missing (i.e., a second or third review).

If you're worried about that for now, though, maybe we can bump to 2+ approving reviews and adjust from there ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

the PR from a Contributor that is not an author of the PR. The review
needs to be made after the last commit in the PR (equivalent to
[Stale review dismissal](https://help.github.com/articles/enabling-required-reviews-for-pull-requests/)
option on GitHub).
1. Does not feature any [Reviews that Request changes](https://help.github.com/articles/about-required-reviews-for-pull-requests/).
1. Does not feature "WIP" in the title (Work in Progress).
1. Passes all automated tests.
1. Any Contributor can Review a PR and Request changes. If a Contributor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are Contributor, Review and Request all capitalised for a reason? Because they have particular meaning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I am not married to this particular capitalization. Please propose a different capitalization if you find it more clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of capitalising words that are in the definitions section at the top.... and I also like the idea of defining Review and Request but I'm not super sure I could do it at the moment...

(This isn't super helpful... maybe it could be an issue for future consideration?)

Request changes they need to provide an explanation what changes
should be added and justification of their importance. Reviews requesting
changes can also we used to request more time to review a PR.
chrisgorgo marked this conversation as resolved.
Show resolved Hide resolved
1. A Contributor that Requested changes can Dismiss their own review or Approve
changes added by the Contributor who opened the PR.
1. If the author of a PR and Contributor who provided Review that Requests
changes cannot find a solution that would lead to the Contributor dismissing
their review or accepting the changes the Review can be Dismissed with a
vote. Rules governing voting:
1. A Vote can be triggered by any Contributor, but only after 5 working days
from the time a Review Requesting Changes has been raised and in case a
Vote has been triggered previously no sooner than 15 working days since
its conclusion.
1. Only Contributors can vote, each contributor gets one vote.
1. A Vote ends after 5 working days or when all Contributors have voted
(whichever comes first).
1. A Vote freezes the PR - no new commits or Reviews Requesting changes can
be added to it while a vote is ongoing. If a commit is accidentally made
during that period it should be reverted.
1. The quorum for a Vote is 30% of all Contributors.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All contributors over time, or just for the PR?

If it's over time, this may become infeasible as people move on in their careers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All contributors. Your concern is indeed valid, but over time we can adjust this percentage. I previously considered an option for a quorum consisting of recent commit authors, but that did not count other contributors to the BIDS ecosystem, so I left it out.

1. The outcome of the vote is decided based on a simple majority.

## Comments

1. There are no restrictions on how the content of the PR is prepared. For
example it is perfectly fine for a PR to consist of content developed by a
group of experts over an extended period of time via in person meetings and
online collaborations using a Google Document.
1. To facilitate triaging of incoming PR you can subscribe to
notifications for new PRs proposing changes to specific files. To do this
add your Github name next to the file you want to subscribe to in the
[CODEOWNERS](CODEOWNERS). This way you will be ask to review each relevant
PR. Please mind that lack of your review will not prevent the PR from being
merged so if you think the PR needs you attention review it promptly or
chrisgorgo marked this conversation as resolved.
Show resolved Hide resolved
request more time via Request changes.
1. Releases are triggered the same way as any other change - via a PR.