|
| 1 | +.. _core_dev: |
| 2 | + |
| 3 | +Core Developer Guide |
| 4 | +==================== |
| 5 | + |
| 6 | +As a core developer, you should continue making pull requests |
| 7 | +in accordance with the :ref:`contributor_guide`. |
| 8 | +You are responsible for shepherding other contributors through the review process. |
| 9 | +You should be familiar with our :ref:`mission_and_values`. |
| 10 | +You also have the ability to merge or approve other contributors' pull requests. |
| 11 | +Much like nuclear launch keys, it is a shared power: you must merge *only after* |
| 12 | +another core developer has approved the pull request, *and* after you yourself have carefully |
| 13 | +reviewed it. (See `Reviewing`_ and especially `Merge Only Changes You |
| 14 | +Understand`_ below.) To ensure a clean git history, use GitHub's |
| 15 | +`Squash and Merge <https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/merging-a-pull-request#merging-a-pull-request-on-github>`__ |
| 16 | +feature to merge, unless you have a good reason not to do so. |
| 17 | + |
| 18 | +Reviewing |
| 19 | +--------- |
| 20 | + |
| 21 | +How to Conduct A Good Review |
| 22 | +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
| 23 | + |
| 24 | +*Always* be kind to contributors. Nearly all of NetworkX is |
| 25 | +volunteer work, for which we are tremendously grateful. Provide |
| 26 | +constructive criticism on ideas and implementations, and remind |
| 27 | +yourself of how it felt when your own work was being evaluated as a |
| 28 | +novice. |
| 29 | + |
| 30 | +NetworkX strongly values mentorship in code review. New users |
| 31 | +often need more handholding, having little to no git |
| 32 | +experience. Repeat yourself liberally, and, if you don’t recognize a |
| 33 | +contributor, point them to our development guide, or other GitHub |
| 34 | +workflow tutorials around the web. Do not assume that they know how |
| 35 | +GitHub works (e.g., many don't realize that adding a commit |
| 36 | +automatically updates a pull request). Gentle, polite, kind |
| 37 | +encouragement can make the difference between a new core developer and |
| 38 | +an abandoned pull request. |
| 39 | + |
| 40 | +When reviewing, focus on the following: |
| 41 | + |
| 42 | +1. **API:** The API is what users see when they first use |
| 43 | + NetworkX. APIs are difficult to change once released, so |
| 44 | + should be simple, `functional |
| 45 | + <https://en.wikipedia.org/wiki/Functional_programming>`__ (i.e. not |
| 46 | + carry state), consistent with other parts of the library, and |
| 47 | + should avoid modifying input variables. Please familiarize |
| 48 | + yourself with the project's :ref:`deprecation_policy`. |
| 49 | + |
| 50 | +2. **Documentation:** Any new feature should have a gallery |
| 51 | + example that not only illustrates but explains it. |
| 52 | + |
| 53 | +3. **The algorithm:** You should understand the code being modified or |
| 54 | + added before approving it. (See `Merge Only Changes You |
| 55 | + Understand`_ below.) Implementations should do what they claim, |
| 56 | + and be simple, readable, and efficient. |
| 57 | + |
| 58 | +4. **Tests:** All contributions to the library *must* be tested, and |
| 59 | + each added line of code should be covered by at least one test. Good |
| 60 | + tests not only execute the code, but explores corner cases. It is tempting |
| 61 | + not to review tests, but please do so. |
| 62 | + |
| 63 | +Other changes may be *nitpicky*: spelling mistakes, formatting, |
| 64 | +etc. Do not ask contributors to make these changes, and instead |
| 65 | +make the changes by `pushing to their branch |
| 66 | +<https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork>`__, |
| 67 | +or using GitHub’s `suggestion |
| 68 | +<https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/commenting-on-a-pull-request>`__ |
| 69 | +`feature |
| 70 | +<https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/incorporating-feedback-in-your-pull-request>`__. |
| 71 | +(The latter is preferred because it gives the contributor a choice in |
| 72 | +whether to accept the changes.) |
| 73 | + |
| 74 | +Our default merge policy is to squash all PR commits into a single |
| 75 | +commit. Users who wish to bring the latest changes from ``main`` |
| 76 | +into their branch should be advised to merge, not to rebase. Even |
| 77 | +when merge conflicts arise, don’t ask for a rebase unless you know |
| 78 | +that a contributor is experienced with git. Instead, rebase the branch |
| 79 | +yourself, force-push to their branch, and advise the contributor on |
| 80 | +how to force-pull. If the contributor is no longer active, you may |
| 81 | +take over their branch by submitting a new pull request and closing |
| 82 | +the original. In doing so, ensure you communicate that you are not |
| 83 | +throwing the contributor's work away! You should use GitHub's |
| 84 | +``Co-authored-by:`` keyword for commit messages to credit the |
| 85 | +original contributor. |
| 86 | + |
| 87 | + |
| 88 | +Please add a note to a pull request after you push new changes; GitHub |
| 89 | +may not send out notifications for these. |
| 90 | + |
| 91 | +Merge Only Changes You Understand |
| 92 | +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
| 93 | + |
| 94 | +*Long-term maintainability* is an important concern. Code doesn't |
| 95 | +merely have to *work*, but should be *understood* by multiple core |
| 96 | +developers. Changes will have to be made in the future, and the |
| 97 | +original contributor may have moved on. |
| 98 | + |
| 99 | +Therefore, *do not merge a code change unless you understand it*. Ask |
| 100 | +for help freely: we have a long history of consulting community |
| 101 | +members, or even external developers, for added insight where needed, |
| 102 | +and see this as a great learning opportunity. |
| 103 | + |
| 104 | +While we collectively "own" any patches (and bugs!) that become part |
| 105 | +of the code base, you are vouching for changes you merge. Please take |
| 106 | +that responsibility seriously. |
| 107 | + |
| 108 | +Closing issues and pull requests |
| 109 | +-------------------------------- |
| 110 | + |
| 111 | +Sometimes, an issue must be closed that was not fully resolved. This can be |
| 112 | +for a number of reasons: |
| 113 | + |
| 114 | +- the person behind the original post has not responded to calls for |
| 115 | + clarification, and none of the core developers have been able to reproduce |
| 116 | + their issue; |
| 117 | +- fixing the issue is difficult, and it is deemed too niche a use case to |
| 118 | + devote sustained effort or prioritize over other issues; or |
| 119 | +- the use case or feature request is something that core developers feel |
| 120 | + does not belong in NetworkX, |
| 121 | + |
| 122 | +among others. Similarly, pull requests sometimes need to be closed without |
| 123 | +merging, because: |
| 124 | + |
| 125 | +- the pull request implements a niche feature that we consider not worth the |
| 126 | + added maintenance burden; |
| 127 | +- the pull request implements a useful feature, but requires significant |
| 128 | + effort to bring up to NetworkX's standards, and the original |
| 129 | + contributor has moved on, and no other developer can be found to make the |
| 130 | + necessary changes; or |
| 131 | +- the pull request makes changes that do not align with our values, such as |
| 132 | + increasing the code complexity of a function significantly to implement a |
| 133 | + marginal speedup, |
| 134 | + |
| 135 | +among others. |
| 136 | + |
| 137 | +All these may be valid reasons for closing, but we must be wary not to alienate |
| 138 | +contributors by closing an issue or pull request without an explanation. When |
| 139 | +closing, your message should: |
| 140 | + |
| 141 | +- explain clearly how the decision was made to close. This is particularly |
| 142 | + important when the decision was made in a community meeting, which does not |
| 143 | + have as visible a record as the comments thread on the issue itself; |
| 144 | +- thank the contributor(s) for their work; and |
| 145 | +- provide a clear path for the contributor or anyone else to appeal the |
| 146 | + decision. |
| 147 | + |
| 148 | +These points help ensure that all contributors feel welcome and empowered to |
| 149 | +keep contributing, regardless of the outcome of past contributions. |
| 150 | + |
| 151 | +Further resources |
| 152 | +----------------- |
| 153 | + |
| 154 | +As a core member, you should be familiar with community and developer |
| 155 | +resources such as: |
| 156 | + |
| 157 | +- Our :ref:`contributor_guide` |
| 158 | +- Our :ref:`code_of_conduct` |
| 159 | +- `PEP8 <https://www.python.org/dev/peps/pep-0008/>`__ for Python style |
| 160 | +- `PEP257 <https://www.python.org/dev/peps/pep-0257/>`__ and the `NumPy |
| 161 | + documentation |
| 162 | + guide <https://numpy.org/doc/stable/docs/howto_document.html>`__ |
| 163 | + for docstrings. (NumPy docstrings are a superset of PEP257. You |
| 164 | + should read both.) |
| 165 | +- The NetworkX `tag on |
| 166 | + StackOverflow <https://stackoverflow.com/questions/tagged/networkx>`__ |
| 167 | +- Our `mailing |
| 168 | + list <http://groups.google.com/group/networkx-discuss/>`__ |
| 169 | + |
| 170 | +You are not required to monitor all of the social resources. |
0 commit comments