improvements from feedback 2

This commit is contained in:
Eric Sagnes 2016-10-01 19:02:09 +09:00
parent 7ad26bdc6e
commit bf86f9f016

View file

@ -6,13 +6,13 @@
<title>Reviewing contributions</title>
<para>The nixpkgs projects receives a fairly high number of contributions via github pull-requests. Reviewing and approving these is an important task and a way to contribute to the project.</para>
<para>The nixpkgs projects receives a fairly high number of contributions via GitHub pull-requests. Reviewing and approving these is an important task and a way to contribute to the project.</para>
<para>The high change rate of nixpkgs make any pull request that is open for long enough subject to conflicts that will require extra work from the submitter or the merger. Reviewing pull requests in a timely manner and being responsive to the comments is the key to avoid these. Github provides sort filters that can be used to see the <link xlink:href="https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc">most recently</link> and the <link xlink:href="https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-asc">least recently</link> updated pull-requests.</para>
<para>When reviewing a pull request, please always be nice and polite. Controversial changes can lead to controversial opinions, but it is important to respect every community members and their work.</para>
<para>Github provides emoji, they are a simple and quick way to provide feedback to pull-requests or any comments. The thumb-down emoji should be used with care and if possible accompanied with some explanations so the submitter has directions to improve his contribution.</para>
<para>GitHub provides reactions, they are a simple and quick way to provide feedback to pull-requests or any comments. The thumb-down reaction should be used with care and if possible accompanied with some explanations so the submitter has directions to improve his contribution.</para>
<para>Pull-requests reviews should include a list of what has been reviewed in a comment, so other reviewers and mergers can know the state of the review.</para>
@ -35,7 +35,7 @@
<listitem><para>Ensure that the commit text is fitting the guidelines.</para></listitem>
<listitem><para>Ensure that the package maintainers are notified.</para>
<itemizedlist>
<listitem><para>mention-bot usually notify github users based on the submitted changes, but it can happen that it misses some of the package maintainers.</para></listitem>
<listitem><para>mention-bot usually notify GitHub users based on the submitted changes, but it can happen that it misses some of the package maintainers.</para></listitem>
</itemizedlist>
</listitem>
<listitem><para>Ensure that the meta field contains correct information.</para>
@ -52,8 +52,8 @@
<screen>
$ git remote add channels https://github.com/NixOS/nixpkgs-channels.git <co xml:id='reviewing-rebase-1' />
$ git fetch channels nixos-unstable <co xml:id='reviewing-rebase-2' />
$ git fetch origin pull/PRNUMBER/head:PRNUMBER <co xml:id='reviewing-rebase-3' />
$ git rebase --onto nixos-unstable PRNUMBER <co xml:id='reviewing-rebase-4' />
$ git fetch origin pull/PRNUMBER/head <co xml:id='reviewing-rebase-3' />
$ git rebase --onto nixos-unstable BASEBRANCH FETCH_HEAD <co xml:id='reviewing-rebase-4' />
</screen>
<calloutlist>
<callout arearefs='reviewing-rebase-1'>
@ -63,7 +63,7 @@ $ git rebase --onto nixos-unstable PRNUMBER <co xml:id='reviewing-rebase-4' />
<para>Fetching the nixos-unstable branch.</para>
</callout>
<callout arearefs='reviewing-rebase-3'>
<para>Fetching the pull-request changes, <varname>PRNUMBER</varname> is the number at the end of the pull-request title.</para>
<para>Fetching the pull-request changes, <varname>PRNUMBER</varname> is the number at the end of the pull-request title and <varname>BASEBRANCH</varname> the base branch of the pull-request.</para>
</callout>
<callout arearefs='reviewing-rebase-3'>
<para>Rebasing the pull-request changes to the nixos-unstable branch.</para>
@ -124,7 +124,7 @@ $ nix-shell -p nox --run "nox-review -k pr PRNUMBER"
<listitem><para>Ensure the package source.</para>
<itemizedlist>
<listitem><para>Mirrors urls should be used when available.</para></listitem>
<listitem><para>The most appropriate function should be used (e.g. packages from github should use <literal>fetchFromGithub</literal>).</para></listitem>
<listitem><para>The most appropriate function should be used (e.g. packages from GitHub should use <literal>fetchFromGitHub</literal>).</para></listitem>
</itemizedlist>
</listitem>
<listitem><para>Building the package locally.</para></listitem>
@ -170,7 +170,7 @@ $ nix-shell -p nox --run "nox-review -k pr PRNUMBER"
</listitem>
<listitem><para>Ensure that the module maintainers are notified.</para>
<itemizedlist>
<listitem><para>Mention-bot notify github users based on the submitted changes, but it can happen that it miss some of the package maintainers.</para></listitem>
<listitem><para>Mention-bot notify GitHub users based on the submitted changes, but it can happen that it miss some of the package maintainers.</para></listitem>
</itemizedlist>
</listitem>
<listitem><para>Ensure that the module tests, if any, are succeeding.</para></listitem>
@ -278,7 +278,13 @@ The main reviewers for a topic can be hard to find as there is no list, but chec
<para>TODO: add the procedure to request merging rights.</para>
<!--
The following paragraph about how to deal with unactive contributors is just a
proposition and should be modified to what the community agrees to be the right
policy.
<para>Please note that contributors with commit rights unactive for more than three months will have their commit rights revoked.</para>
-->
<para>In a case a contributor leaves definitively the Nix community, he should create an issue or notify the mailing list with references of packages and modules he maintains so the maintainership can be taken over by other contributors.</para>