Skip to content

Commit a339b09

Browse files
authored
Merge pull request #58 from m-miedema/DOC/CGuide
Migrating existing Contributor Guide to wiki-style format
2 parents 5c70419 + e8927e3 commit a339b09

19 files changed

+583
-1147
lines changed

docs/community/contributor-guide.md

Lines changed: 0 additions & 560 deletions
This file was deleted.

docs/community/contributorfile.md

Lines changed: 0 additions & 572 deletions
This file was deleted.
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
## Contribution workflow
2+
3+
There are many descriptions of a good contribution workflow out there.
4+
For instance, we suggest to have a look at [tedana's workflow](https://github.com/ME-ICA/tedana/blob/master/CONTRIBUTING.md#making-a-change).
5+
At `physiopy`, we follow a very similar workflow. The only three
6+
differences are:
7+
8+
- If you see an open issue that you would like to work on, check if it
9+
is assigned. If it is, ask the assignee if they need help or want to
10+
be substituted before starting to work on it.
11+
- We ask you to test the code locally before merging it, and then, if
12+
possible, write some automatic tests for the code to be run in our
13+
Continuous Integration! Check the testing section below to know
14+
more.
15+
- We suggest opening a draft PR as soon as you can - so it's easier
16+
for us to help you!
17+
18+
You can find more in-detail description of this process in the following sections of the guide.
Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
## Labels
2+
3+
The current list of labels are
4+
[here](https://github.com/physiopy/phys2bids/labels). They can be used
5+
for **Issues**, **PRs**, or both. We use
6+
[auto](https://github.com/intuit/auto) to automate our semantic
7+
versioning and Pypi upload, so **it's extremely important to use the
8+
right PR labels**!
9+
10+
### Issue & PR labels
11+
12+
- ![Documentation](https://img.shields.io/badge/-Documentation-1D70CF?style=flat-square) Improvements or additions to documentation. This
13+
category includes (but is not limited to) docs pages, docstrings,
14+
and code comments.
15+
16+
- ![Duplicate](https://img.shields.io/badge/-Duplicate-CFD3D7?style=flat-square) Whatever this is, it exists already! Maybe it's a closed
17+
Issue/PR, that should be reopened.
18+
19+
- ![Enhancement](https://img.shields.io/badge/-Enhancement-A2EEEF?style=flat-square) New features added or requested. This normally goes with a `minormod` label for PRs.
20+
21+
- ![Outreach](https://img.shields.io/badge/-Outreach-0E8A16?style=flat-square) As part of the scientific community, we care about outreach. Check the relevant section about it, but know that this
22+
Issue/PR contains information or tasks about abstracts, talks,
23+
demonstrations, papers.
24+
25+
- ![Paused](https://img.shields.io/badge/-Paused-F7C38C?style=flat-square) Issue or PR should not be worked on until the resolution of other issues or PRs.
26+
27+
- ![released](https://img.shields.io/badge/-released-ffffff?style=flat-square) This Issue or PR has been released.
28+
29+
- ![Testing](https://img.shields.io/badge/-Testing-FFB5B4?style=flat-square) This is for testing features, writing tests or producing testing code. Both user testing and CI testing!
30+
31+
- ![Urgent](https://img.shields.io/badge/-Urgent-FFF200?style=flat-square) If you don't know where to start, start here! This is probably related to a milestone due soon!
32+
33+
### Issue-only labels
34+
35+
- ![BrainHack](https://img.shields.io/badge/-BrainHack-000000?style=flat-square) This issue is suggested for BrainHack participants!
36+
37+
- ![Bug](https://img.shields.io/badge/-Bug-D73A4A?style=flat-square) Something isn't working. It either breaks the code or has an
38+
unexpected outcome.
39+
40+
- ![Community](https://img.shields.io/badge/-Community-E2C7FC?style=flat-square) This issue contains information about the `physiopy`
41+
community (e.g. the next developer call)
42+
43+
- ![Discussion](https://img.shields.io/badge/-Discussion-1C778C?style=flat-square) Discussion of a concept or implementation. These Issues
44+
are prone to be open ad infinitum. Jump in the conversation if you
45+
want!
46+
47+
- ![Good first issue](https://img.shields.io/badge/-Good%20first%20issue-4E2A84?style=flat-square) Good for newcomers. These issues calls for a
48+
**fairly** easy enhancement, or for a change that helps/requires
49+
getting to know the code better. They have educational value, and
50+
for this reason, unless urgent, experts in the topic should refrain
51+
from closing them - but help newcomers closing them.
52+
53+
- ![Hacktoberfest](https://img.shields.io/badge/-Hacktoberfest-FF7518?style=flat-square) Dedicated to the hacktoberfest event, so that people
54+
can help and feel good about it (and show it with a T-shirt!).
55+
**Such commits will not be recognised in the all-contributor table,
56+
unless otherwise specified**.
57+
58+
- ![Help wanted](https://img.shields.io/badge/-Help%20wanted-57DB1A?style=flat-square) Extra attention is needed here! It's a good place to have a look!
59+
60+
- ![Refactoring](https://img.shields.io/badge/-Refactoring-9494FF?style=flat-square) Improve nonfunctional attributes. Which means rewriting
61+
the code or the documentation to improve performance or just because
62+
there's a better way to express those lines. It might create a
63+
`majormod` PR.
64+
65+
- ![Question](https://img.shields.io/badge/-Question-D876E3?style=flat-square) Further information is requested, from users to
66+
developers. Try to respond to this!
67+
68+
- ![Wontfix](https://img.shields.io/badge/-Wontfix-ffffff?style=flat-square) This will not be worked on, until further notice.
69+
70+
### PR-only labels
71+
72+
#### Labels for semantic release and changelogs
73+
74+
- ![BugFIX](https://img.shields.io/badge/-BugFIX-D73A4A?style=flat-square) These PRs close an issue labelled `Bug`. They also increase
75+
the semantic versioning for fixes (+0.0.1).
76+
77+
- ![dependencies](https://img.shields.io/badge/-dependencies-0366D6?style=flat-square) Pull requests that update a dependency file
78+
79+
- ![Documentation](https://img.shields.io/badge/-Documentation-1D70CF?style=flat-square) See above. This PR won't trigger a release, but it will be reported in the changelog.
80+
81+
- ![Majormod](https://img.shields.io/badge/-Majormod-05246D?style=flat-square) These PRs call for a new major release (+1.0.0). This
82+
means that the PR is breaking backward compatibility.
83+
84+
- ![Minormod](https://img.shields.io/badge/-Minormod-05246D?style=flat-square) This PR generally closes an `Enhancement` issue. It increments the minor version (0.+1.0)
85+
86+
- ![Minormod-breaking](https://img.shields.io/badge/-Minormod&ndash;breaking-05246D?style=flat-square) This label should be used during development stages (<1.0.0) only. These PRs call for a new minor release during development (0.+1.0) that **will** break backward compatibility.
87+
88+
- ![Internal](https://img.shields.io/badge/-Internal-ffffff?style=flat-square) This PR contains changes to the internal API. It won't
89+
trigger a release, but it will be reported in the changelog.
90+
91+
- ![Testing](https://img.shields.io/badge/-Testing-FFB5B4?style=flat-square) See above. This PR won't trigger a release, but it will be
92+
reported in the changelog.
93+
94+
- ![Skip release](https://img.shields.io/badge/-Skip%20release-ffffff?style=flat-square) This PR will **not** trigger a release.
95+
96+
- ![Release](https://img.shields.io/badge/-Release-ffffff?style=flat-square) This PR will force the trigger of a release.
97+
98+
#### Other labels
99+
100+
- ![Invalid](https://img.shields.io/badge/-Invalid-960018?style=flat-square): These PRs don't seem right. They actually seem so not
101+
right that they won't be further processed. This label invalidates a
102+
Hacktoberfest contribution. If you think this is wrong, start a
103+
discussion in the relevant issue (or open one if missing). Reviewers
104+
are asked to give an explanation for the use of this label.
105+
106+
### Good First Issues
107+
108+
Good First Issues are issues that are either very simple, or that help
109+
the contributor get to know the programs or the languages better. We use
110+
it to help contributors with less experience to learn and familiarise
111+
with Git, GitHub, Python3, and physiology. We invite more expert
112+
contributors to avoid those issues, leave them to beginners and possibly
113+
help them out in the resolution of the issue. However, if the issue is
114+
left unassigned or unattended for long, and it's considered important or
115+
urgent, anyone can tackle it.
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
## Issues and Milestones
2+
3+
At `physiopy`, we use Issues and Milestones to keep track of and
4+
organise our workflow. **Issues** describe pieces of work that need to
5+
be completed to move the project forwards. We try to keep them as simple
6+
and clear as possible: an issue should describe a unitary, possibly
7+
small piece of work (unless it's about refactoring). Don't be scared of
8+
opening many issues at once, if it makes sense! Just check that what
9+
you're proposing is not listed in a previous issue (open or closed) yet
10+
(we don't like doubles). Issues get labelled. That helps the
11+
contributors to know what they're about. Check the label list to know
12+
what types are there, and use them accordingly! Issues can also be
13+
**assigned**. If you want to work on an assigned issue, ask permission
14+
first! - **Milestones** set the higher level workflow. They sketch
15+
deadlines and important releases. Issues are assigned to these
16+
milestones by the maintainers. If you feel that an issue should be
17+
assigned to a specific milestone but the maintainers have not done so,
18+
discuss it in the issue chat or in Gitter! We might have just missed it,
19+
or we might not (yet) see how it aligns with the overall project
20+
structure/milestone.
Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
## Pull Requests
2+
3+
To improve understanding pull requests "at a glance" and use the power
4+
of `auto`, we use the labels listed above. Multiple labels can be
5+
assigned to a PR - in fact, all those that you think are relevant. We
6+
strongly advise to keep the changes you're introducing with your PR
7+
limited to your original goal. Adding to the scope of your PR little
8+
style corrections or code refactoring here and there in the code that
9+
you're already modifying is a great help, but when they become too much
10+
(and they are not relevant to your PR) they risk complicating the nature
11+
of the PR and the reviewing process. It is much better to open another
12+
PR with the objective of doing such corrections! Moreover, if you're
13+
tempted to assign more than one label that would trigger a release (e.g.
14+
bug and minormod or bug and majormod, etc.), you might want to split your PR
15+
instead. When opening a pull request, assign it at least one label.
16+
17+
We encourage you to open a PR as soon as possible - even before you
18+
finish working on them. This is useful especially to you - so that you
19+
can receive comments and suggestions early on, rather than having to
20+
process a lot of comments in the final review step! However, if it's an
21+
incomplete PR, please open a **Draft PR**. That helps us process PRs by
22+
knowing which one to have a look first - and how picky to be when doing
23+
so.
24+
25+
Reviewing PRs is a time consuming task and it can be stressful for both
26+
the reviewer and the author. Avoiding wasting time and the need of
27+
little fixes - such as fixing grammar mistakes and typos, styling code,
28+
or adopting conventions - is a good start for a successful (and quick)
29+
review. Before graduating a Draft PR to a PR ready for review, please
30+
check that:
31+
32+
- You did all you wanted to include in your PR. If at a later stage
33+
you realize something is missing and it's not a minor thing, you
34+
will need to open a new PR.
35+
- If your contribution contains code or tests, you ran and passed all
36+
of the tests locally with [pytest](#automatic-testing).
37+
- If you're writing documentation, you built it locally with
38+
sphinx and the format is what you intended.
39+
- Your code is harmonious with the rest of the code - no repetitions
40+
of any sort!
41+
42+
Your code respects the [adopted Style](#style-guide), especially if:
43+
44+
- Your code is lintered adequately and respects the [PEP8](https://www.python.org/dev/peps/pep-0008/) convention.
45+
- Your docstrings follow the [numpydoc](https://numpydoc.readthedocs.io/en/latest/format.html) convention.
46+
- There are no typos or grammatical mistakes and the text is fluid.
47+
48+
- The code is sufficiently commented and the comments are clear.
49+
50+
- Your PR title is clear enough to be meaningful when appended to the version changelog.
51+
52+
- You have the correct labels.
53+
54+
55+
### Before merging
56+
57+
To be merged, PRs have to:
58+
59+
1. Pass all the CircleCI tests, and possibly all the codecov checks.
60+
2. Have the necessary amount of approving reviews, even if you're a
61+
long time contributor.
62+
63+
Note : You can ask one (or more) contributor to do
64+
that review, if you think they align more with the content of your
65+
PR. You need **one** review for documentation, tests, and small
66+
changes, and **two** reviews for bugs, refactoring and enhancements.
67+
3. Have at least a release-related label (or a `Skip
68+
release` label).
69+
4. Have a short title that clearly explains in one sentence the aim of
70+
the PR.
71+
5. Contain at least a unit test for your contribution, if the PR
72+
contains code (it would be better if it contains an integration or
73+
function test and all the breaking tests necessary). If you're not
74+
confident about writing tests, it is possible to refer to an issue
75+
that asks for the test to be written, or another (Draft) PR that
76+
contains the tests required.
77+
78+
As we're trying to maintain at least 90% code coverage, you're strongly
79+
encouraged to write all of the tests necessary to keep coverage above
80+
that threshold. If coverage drops too low, you might be asked to add
81+
more tests and/or your PR might be rejected. See the [Automatic testing](#automatic-testing) section.
82+
83+
Don't merge your own pull request! That's a task for the main reviewer
84+
of your PR or the project manager. Remember that the project manager
85+
doesn't have to be a reviewer of your PR!
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
## Under development
2+
We're still building this section of the guide, so stay tuned (or help!)
3+
4+
We'd like to provide more information on some of the tools you may use in your contributions to Physiopy such as
5+
6+
### Pre-commit
7+
### CircleCI
8+
### Pytest
9+
### Pypi
10+
### OSF/test data
11+
12+
13+
Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
## Reviewing PRs
2+
3+
Reviewing PRs is an extremely important task in collaborative
4+
development. In fact, it is probably the task that requires the most
5+
time in the development, and it can be stressful for both the reviewer
6+
and the author. Remember that, as a PR Reviewer, you are guaranteeing
7+
that the changes work and integrate well with the rest of the
8+
repository, hence **you are responsible for the quality of the
9+
repository and its next version release**. If they don't integrate
10+
well, later PR reviewers might have to ask for broader changes than
11+
expected.
12+
13+
There are many best practices to review code online, for
14+
instance [this medium blog post](https://medium.com/an-idea/the-code-review-guide-9e793edcd683), but
15+
here are some good rules of thumbs that we need to follow while
16+
reviewing PRs:
17+
18+
- Be **respectful** to the PR authors and be clear in what you are
19+
asking/suggesting - remember that, like you, they are contributing
20+
their spare time and doing their best job!
21+
22+
- If there is a *Draft PR*, you can comment on its development in the
23+
message board or making "Comment" reviews. Don't ask for changes,
24+
and especially, **don't approve the PR**
25+
26+
- If the PR graduated *from Draft to full PR*, check that it follows the
27+
sections [Pull requests](#pull-requests) and [Style Guide](#style-guide) of these
28+
guidelines. If not, invite the author to do so before starting a
29+
review.
30+
- **Don't limit your review to the parts that are changed**. Look at
31+
the entire file, see if the changes fit well in it, and see if the
32+
changes are properly addressed everywhere in the code - in the
33+
documentation, in the tests, and in other functions. Sometimes the
34+
differences reported don't show the full impact of the PR in the
35+
repository!
36+
- If your want to make Pull Requests an educational process, invite
37+
the author of the PR to make changes before actually doing them
38+
yourself. Request changes via comments or in the message board or by
39+
checking out the PR locally, making changes and then submitting a PR
40+
to the author's branch.
41+
- If you decide to use the suggestion tool in reviews, or to start a
42+
PR to the branch under review, please alert the Project Manager.
43+
Bots might automatically assign you contribution types that will
44+
have to be removed (remember, your contribution in this case is
45+
"Reviewer"). Instead of starting a PR to the branch under review,
46+
think about opening a new PR with those modifications (unless they
47+
are needed to pass tests), and alert the Main Reviewer. In any case
48+
**don't commit directly to the branch under review**!
49+
- If you're reviewing documentation, build it locally with [`sphinx-build`](https://www.sphinx-doc.org/en/master/man/sphinx-build.html) command.
50+
- If you're asking for changes, **don't approve the PR**. Approve it
51+
only after everything was sufficiently addressed. Someone else might
52+
merge the PR in taking your word for granted.
53+
- If you are the main reviewer, and the last reviewer required to
54+
approve the PR, merge the PR!
55+
56+
Before approving and/or merging PRs, be sure that:
57+
58+
- All the tests in CircleCI/Azure pass without errors.
59+
- Prefereably, codecov checks pass as well. If they don't, discuss
60+
what to do.
61+
- The title describes the content of the PR clearly enough to be
62+
meaningful on its own - remember that it will appear in the version
63+
changelog!
64+
- The PR has the appropriate labels to trigger the appropriate version
65+
release and update the contributors table.
66+
67+
### Main reviewer
68+
69+
At `physiopy` we use the *Assignees* section of a PR to mark the
70+
**main reviewer** for that PR. The main reviewer is the primary person
71+
responsible **for the quality of the repository and its next version release**,
72+
as well as **for the behaviour of the other reviewers**.
73+
74+
***The main reviewer takes care of the reviewing process of the PR, in particular:***
75+
76+
- Invites the reviewers to finish their review in a relatively
77+
short time.
78+
79+
- Checks that all elements of this document were respected,
80+
especially the part about [Pull Requests](#pull-requests).
81+
82+
- Invites other Reviewers to respect this document, especially
83+
the part about [reviews](#reviewing-prs), helps them in doing
84+
so, and checks that they do.
85+
86+
- If a Reviewer keeps not respecting this document, flags them
87+
to the project manager.
88+
89+
- Decides what to do in case of a coverage decrease (in
90+
*codecov/patch*).
91+
92+
***In case of missing tests or updates to user documentation:***
93+
94+
- Asks for more documentation or tests before approving the
95+
PR, *or*
96+
97+
- Checks that the appropriate issues have been opened to
98+
address the lack of documentation or tests (1 issue per item), *or*
99+
100+
- Double-checks that the title is clear and the labels are correct to
101+
trigger an appropriate `auto` release - feel free to change them.
102+
103+
- Main reviewer **Is the one that is going to merge the PR.**
104+
105+
***After the PR has been merged and a new release has been triggered, checks that:***
106+
107+
- The documentation was updated correctly (if changed).
108+
- The Pypi version of the repository coincides with the new release (if changed).
109+
- New contributors or forms of contributions were correctly added in the README (if changed).
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
## Under development
2+
We're still building this section of the guide, so stay tuned (or help!)
3+
4+

0 commit comments

Comments
 (0)