From 8a28ec62e7a782c63dbadc7ba12fdb96376dbae7 Mon Sep 17 00:00:00 2001 From: smoia Date: Fri, 9 Oct 2020 21:43:52 +0200 Subject: [PATCH 01/28] Update documentation with auto and main reviewer --- docs/contributing.rst | 10 +-- docs/contributorfile.rst | 187 ++++++++++++++++++++++++++++----------- 2 files changed, 140 insertions(+), 57 deletions(-) diff --git a/docs/contributing.rst b/docs/contributing.rst index 9b277079b..b79560961 100644 --- a/docs/contributing.rst +++ b/docs/contributing.rst @@ -9,12 +9,11 @@ First of all: thank you! Contributions can be made in different ways, not only code! As we follow the `all-contributors`_ specification, any contribution will be recognised accordingly. -The first thing you might want to do, is to have a look at the `contributor guide `_ page as well as the `code of conduct `_. +Follow these steps to get started: -The second thing is to check you have ``git`` and ``pip`` installed in your system. - -The third thing is to install ``phys2bids`` as a developer. -This will let you run the program with the latest modification, without requiring you to re-install it every time. +1. Have a look at the `contributor guide `_ page as well as the `code of conduct `_. +2. Make sure that you have a GitHub account. You can set up a `free GitHub account `_; here are some `instructions `_. +3. If you intend to contribute code and/or use the ``physiopy`` packages in any way, check that you have ``git`` and ``pip`` installed on your system. Then install ``phys2bids`` as a developer. This will let you run the program with the latest modification, without requiring to re-install it every time. .. _`all-contributors`: https://github.com/all-contributors/all-contributors @@ -23,7 +22,6 @@ This will let you run the program with the latest modification, without requirin If it is, you might need to use ``pip`` instead of ``pip3``, although some OSs do adopt ``pip3`` anyway. If you want to check, type ``python --version`` in a terminal. - Linux and mac developer installation ------------------------------------ diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index 5997c661b..127859c75 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -3,9 +3,9 @@ ================================= How to contribute to ``physiopy`` ================================= -Welcome to the physiopy organisation! It’s great news you’re thinking about contributing! -Working with many people from many different places is great, but sometimes this means the code can become messy due to the many different ways a contribution can be made. For this reason, we have set up some guidelines for contributions - to help you get along ASAP! -Before you start you'll need to set up a free `GitHub `_ account and sign in. Here are some `instructions `_. +Welcome to the ``physiopy`` organisation! It’s great news that you’re thinking about contributing! + +Working with many people from many different places is great, but sometimes this means that code can become messy due to the many different ways a contribution can be made. For this reason, we have set up some guidelines for contributions - to help you get involved ASAP! If you lack knowledge in python development / github use / physiological data handling, don’t be scared! Try to jump in anyway. Most of the original contributors learned these things exactly this way - jumping in and hoping to fall in the right way without breaking too many bones. Do you want to jump in but don’t exactly know where/how? You can drop a few lines in `gitter `_, so we can help you find something that suits you! Already know what you're looking for in this guide? Jump to the following sections: @@ -18,28 +18,30 @@ Already know what you're looking for in this guide? Jump to the following sectio - `Contributing with test files <#testfile>`_ - `Contributing documentation through GitHub <#documenting>`_ - `Contributing code through GitHub <#code>`_ + - `Contributing with Pull Requests Review <#reviews>`_ - `Issues and Milestones <#issuesmilestones>`_ - `Labels <#labeltypes>`_ + - `Issues & PRs labels <#issueprlabels>`_ - `Issues labels <#issuelabel>`_ - `PRs labels <#prlabel>`_ - - `Issues & PRs labels <#issueprlabels>`_ - `Good First Issues <#g1i>`_ - `Contribution workflow <#workflow>`_ - `Pull Requests <#pr>`_ +- `Reviewing PRs <#reviewing>`_ - `Style Guide <#styling>`_ - `Automatic Testing <#testing>`_ - `Recognizing contributors <#recognising>`_ .. _aims: -Aims of physiopy ----------------- -physiopy is a **very** young project developed by a bunch of researchers from the two different sides of the Atlantic Ocean (for now). -Our main goal is to help collect, analyse and share physiological data interfacing with (MRI) neuroimaging. We’re trying to do so by: +Aims of ``physiopy`` +-------------------- +``physiopy`` is a **very** young project developed by a bunch of researchers from the two different sides of the Atlantic Ocean (for now). +Our main goal is to help collect, analyse and share physiological data, interfacing with (MRI) neuroimaging. We’re trying to do so by: -1. Write packages to make a user-friendly pipeline to deal with physiological data. -2. Organising a documentation containing tips and strategies on how to collect such data and use our packages. -3. Write packages that take into account the use of such data in combination with neuroimaging (MRI) - by getting everything ready for that analysis. +1. Writing packages to make a user-friendly pipeline to deal with physiological data. +2. Writing packages that take into account the use of this physiological data in combination with neuroimaging (MRI) analysis. +3. Providing documentation containing tips and strategies on how to collect such data and use our packages. 4. Help set a standard for these data, albeit without forcing users to use it. 5. Be an excuse for educational purposes on topics like Git/GitHub, Python3, physiology and related tools/topics. @@ -47,8 +49,8 @@ Our main goal is to help collect, analyse and share physiological data interfaci Joining the conversation ------------------------ -We’re trying to keep all the conversation related to the project development in GitHub `issues `_. -We maintain a `gitter chat room `_ for more informal conversations and general project updates. +We’re trying to keep all the conversation related to the project development in GitHub `issues `_. +We maintain a `gitter chat room `_ for more informal conversations and general project updates. We also have a dev call once a month - specifically the second Thursday of the month! If you want to participate, drop a line in gitter! When interacting in the common channels, please adhere to our `code of conduct `_. @@ -62,13 +64,13 @@ Contributions Contributing with small documentation changes ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ If you are new to GitHub and just have a small documentation change recommendation (such as: typos detection, small improvements in the content, ...), please open an issue in the relative project, and label it with the “Documentation” label. -Chances are those types of changes are easily doable with the online editor, which means you can do them, or ask for help from the developers! +Chances are those types of changes are easily doable with GitHub's online editor, which means you can do them, or ask for help from the developers! .. _usertests: Contributing with User testing ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Another, non-coding friendly way to contribute to physiopy is by testing the packages. +Another, non-coding friendly way to contribute to ``physiopy`` is by testing the packages. There are different kinds of tests, but to simplify things you can think mainly about automatic tests and user tests. To know more about **Automatic tests**, you can read the `testing section <#testing>`_. **User testing** are warm, human, emotional and opinionated tests that not only check that the code is doing what it needs to do, but also whether there’s a better way to do it - namely better reports, clearer screen outputs, warnings and exceptions, unexpected bugs that have to be corrected. @@ -78,10 +80,10 @@ If you want to perform one, open an issue on GitHub or drop a comment in Gitter, Contributing with test files ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -At physiopy we always try to imagine and support every possible setting out there. However, our imagination has a limit - but if you think our packages should process a specific format/setting that you have, we’re more than glad to do so! +At ``physiopy`` we always try to imagine and support every possible setting out there. However, our imagination has a limit - but if you think our packages should process a specific format/setting that you have, we’re more than glad to do so! To make it happen, we need an example of the file we want to process, so you will have to share it with us (and the rest of the world)! The contribution can be a full file of data that you already acquired, a part of that file (pay attention to what is the minimum you need to share!), or mock data. The file contribution should come with a json file of the same name that contains the necessary information to run ``phys2bids`` on that file contribution. There is a `json blueprint in OSF `_, you can download it and adapt it. Note that the frequency list **has to be expressed in Hz** as an integer or float. -To contribute with a test file, open an Issue in GitHub and label it with *Test*. We’ll help you add the file in our +To contribute with a test file, open an Issue in GitHub and label it with *Test*. We’ll help you add the file in our `OSF `_ space. We’re extremely grateful for this type of contribution - so grateful that we asked allcontributors to add a dedicated category! @@ -89,25 +91,33 @@ We’re extremely grateful for this type of contribution - so grateful that we a Contributing documentation through GitHub ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -We use `readthedocs `_ to create our documentation. Every contribution is welcome - and it follows the steps of a code contribution. +We use `readthedocs `_ to create our documentation. Every contribution is welcome and it follows the same steps as a code contribution, explained below. .. _code: Contributing code through GitHub ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -This section covers 90% of the contributions a project like physiopy receives - code, documentation and tests. -The best way to make this kind of contributions, in a nutshell, would be: +This section covers 90% of the contributions a project like ``physiopy`` receives - code, documentation and tests. +The best way to make this kind of contributions, in a nutshell, is to: 1. Open an issue with the intended modifications. 2. Label it, discuss it, (self-)assign it. 3. Open a Pull Request (PR) to resolve the issue and label it. 4. Wait for a review, discuss it or comply, repeat until ready. Issues and PR chats are great to maintain track of the conversation on the contribution. They are based upon GitHub-flavoured `Markdown `_. GitHub has a helpful page on `getting started with writing and formatting Markdown on GitHub `_. +.. _reviews: + +Contributing with Pull Requests Reviews +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +One big challenge in this kind of software development is merging code accurately but without waiting too much time. +For this reason, Reviewers for PRs are more than welcome! It is a task that requires some experience, but it's very necessary! +Read the `related section below <#reviewing>`_ to start! + .. _issuesmilestones: Issues and Milestones --------------------- -At physiopy, we use Issues and Milestones to keep track of and organise our workflow. +At ``physiopy``, we use Issues and Milestones to keep track of and organise our workflow. - **Issues** describe pieces of work that need to be completed to move the project forwards. We try to keep them as simple and clear as possible: an issue should describe a unitary, possibly small piece of work (unless it’s about refactoring). Don’t be scared of opening many issues at once, if it makes sense! Just check that what you’re proposing is not listed in a previous issue (open or closed) yet - we don’t like doubles. Issues get labelled. That helps the contributors to know what they’re about. Check the label list to know what types are there, and use them accordingly! Issues can also be **assigned**. If you want to work on an assigned issue, ask permission first! - **Milestones** set the higher level workflow. They sketch deadlines and important releases. Issues are assigned to these milestones by the maintainers. If you feel that an issue should be assigned to a specific milestone but the maintainers have not done so, discuss it in the issue chat or in Gitter! We might have just missed it, or we might not (yet) see how it aligns with the overall project structure/milestone. @@ -116,12 +126,26 @@ At physiopy, we use Issues and Milestones to keep track of and organise our work Labels ------ The current list of labels are `here `_. They can be used for **Issues**, **PRs**, or both. +We use `auto `_ to automatise our semantic versioning and Pypi upload, so **it's extremely important to use the right PR labels**! + +.. _issueprlabels: + +Issue & PR labels +~~~~~~~~~~~~~~~~~~~ +- Documentation: Improvements or additions to documentation. This category includes (but is not limited to) docs pages, docstrings, and code comments. +- Duplicate: Whatever this is, it exists already! Maybe it’s a closed Issue/PR, that should be reopened. +- Enhancement: New features added or requested. This normally goes with a ``minormod`` label for PRs. +- Outreach: As part of the scientific community, we care about outreach. Check the relevant section about it, but know that this Issue/PR contains information or tasks about abstracts, talks, demonstrations, papers. +- Paused: Issue or PR should not be worked on until the resolution of other issues or PRs. +- Testing: This is for testing features, writing tests or producing testing code. Both user testing and CI testing! +- Urgent: If you don't know where to start, start here! This is probably related to a milestone due soon! .. _issuelabel: -Issues labels -~~~~~~~~~~~~~ +Issue-only labels +~~~~~~~~~~~~~~~~~~ - Bug: Something isn’t working. It either breaks the code or has an unexpected outcome. +- Community: This issue contains information about the `physiopy` community (e.g. the next developer call) - Discussion: Discussion of a concept or implementation. These Issues are prone to be open ad infinitum. Jump in the conversation if you want! - Good first issue: Good for newcomers. These issues calls for a **fairly** easy enhancement, or for a change that helps/requires getting to know the code better. They have educational value, and for this reason, unless urgent, experts in the topic should refrain from closing them - but help newcomers closing them. - Hacktoberfest: Dedicated to the hacktoberfest event, so that people can help and feel good about it (and show it with a T-shirt!). **Such commits will not be recognised in the all-contributor table, unless otherwise specified**. @@ -132,30 +156,29 @@ Issues labels .. _prlabel: -PRs labels -~~~~~~~~~~ -- BugFIX: These PRs close an issue labelled ``bug``. they also increase the semantic versioning for fixes (+0.0.1). -- Invalid: These PRs don't seem right. They actually seem so not right that they won’t be further processed. This label invalidates an Hacktoberfest contribution. If you think this is wrong, start a discussion in the relevant issue (or open one if missing). Reviewers are asked to give an explanation for the use of this label. +PR-only labels +~~~~~~~~~~~~~~~ + +Labels for semantic release and changelogs +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - Majormod: These PRs call for a new major release (+1.0.0). This means that the PR is breaking backward compatibility. - Minormod: These PRs call for a new minor release (0.+1.0). This means that the PR is **not** breaking backward compatibility. +- BugFIX: These PRs close an issue labelled ``bug``. They also increase the semantic versioning for fixes (+0.0.1). +- Internal: This PR contains changes to the internal API. It won't trigger a release, but it will be reported in the changelog. +- Documentation: See above. This PR won't trigger a release, but it will be reported in the changelog. +- Testing: See above. This PR won't trigger a release, but it will be reported in the changelog. +- Skip release: This PR will **not** trigger a release. +- Release: This PR will force the trigger of a release. -.. _issueprlabels: - -Issues & PRs labels -~~~~~~~~~~~~~~~~~~~ -- Documentation: Improvements or additions to documentation. This category includes (but is not limited to) docs pages, docstrings, and code comments. -- Duplicate: Whatever this is, it exists already! Maybe it’s a closed Issue/PR, that should be reopened. -- Enhancement: New features added or requested. This normally goes with a ``minormod`` label for PRs. -- Outreach: As part of the scientific community, we care about outreach. Check the relevant section about it, but know that this Issue/PR contains information or tasks about abstracts, talks, demonstrations, papers. -- Paused: Issue or PR should not be worked on until the resolution of other issues or PRs. -- Testing: This is for testing features, writing tests or producing testing code. Both user testing and CI testing! -- Urgent: If you don't know where to start, start here! This is probably related to a milestone due soon! +Other labels +^^^^^^^^^^^^ +- Invalid: These PRs don't seem right. They actually seem so not right that they won’t be further processed. This label invalidates a Hacktoberfest contribution. If you think this is wrong, start a discussion in the relevant issue (or open one if missing). Reviewers are asked to give an explanation for the use of this label. .. _g1i: Good First Issues ~~~~~~~~~~~~~~~~~ -Good First Issues are issues that are either very simple, or that help knowing the programs or the language better. We use it to help contributors with less experience to learn and familiarise with Git, GitHub, Python3, and physiology. +Good First Issues are issues that are either very simple, or that help the contributor get to know the programs or the languages better. We use it to help contributors with less experience to learn and familiarise with Git, GitHub, Python3, and physiology. We invite more expert contributors to avoid those issues, leave them to beginners and possibly help them out in the resolution of the issue. However, if the issue is left unassigned or unattended for long, and it’s considered important or urgent, anyone can tackle it. .. _workflow: @@ -163,27 +186,88 @@ We invite more expert contributors to avoid those issues, leave them to beginner Contribution workflow --------------------- There are many descriptions of a good contribution workflow out there. For instance, we suggest to have a look at `tedana's workflow `_. -At physiopy, we follow a very similar workflow. The only two differences are: +At ``physiopy``, we follow a very similar workflow. The only three differences are: -- We ask you to test the code locally before merging it, and then, if possible, write some automatic tests for the code to be run in our Continuous Integration! Check the testing section below to know more. -- We suggest opening a draft PR as soon as you can - so it’s easier for us to help you! +- If you see an open issue that you would like to work on, check if it is assigned. If it is, ask the assignee if they need help before starting to work on it. +- We ask you to test the code locally before merging it, and then, if possible, write some automatic tests for the code to be run in our Continuous Integration! Check the testing section below to know more. +- We suggest opening a draft PR as soon as you can - so it’s easier for us to help you! .. _pr: Pull Requests ------------- -To improve understanding pull requests "at a glance", we use the same labels used for issues. Multiple labels can be assigned - just think which ones suit your PR the most! -When opening a pull request, assign it to at least one label. +To improve understanding pull requests "at a glance", we use the labels listed above. Multiple labels can be assigned, but it's a good idea to keep different types of contributions separated, unless they are minimal - for instance, you might want to open a PR for code, one for documentation and one for testing. +In general, if you're tempted to assign more than one label that would trigger a release, you might want to split your PR instead. +When opening a pull request, assign it at least one label. -We encourage you to open a PR as soon as possible - even before you finish working on them. This is useful especially to you - so that you can receive comments and suggestions early on, rather than having to process a lot of comments in the final review step! +We encourage you to open a PR as soon as possible - even before you finish working on them. This is useful especially to you - so that you can receive comments and suggestions early on, rather than having to process a lot of comments in the final review step! However, if it’s an incomplete PR, please open a **Draft PR**. That helps us process PRs by knowing which one to have a look first - and how picky to be when doing so. + +Reviewing PRs is a time consuming task, that can be stressful for both the reviewer and the author. Avoiding wasting time and the need of little fixes - such as fixing grammar mistakes and typos, styling code, or adopting conventions - is a good start for a successful (and quick) review. Before graduating a Draft PR to a PR ready for review, please check that: + +- You did all you wanted to include in your PR. If at a later stage you realise something is missing and it's not a minor thing, you will need to open a new PR. +- If your contribution contains code or tests, you run and passed all the tests locally with `pytest`. +- If you're writing documentation, you built it locally with `sphinx` and the format is what you intended. +- Your code is harmonious with the rest of the code - no repetitions of any sort! +- Your code respects the `adopted Style <#styling>`_, especially: + - Your code is lintered adequately and respects the `PEP8 `_ convention. + - Your docstrings follow the `numpydoc `_ convention. + - There is no grammar mistake or typo and the text is fluid. + - The code is sufficiently commented and the comments are clear. + - Your PR title is clear enough to be meaningful when appended to the version changelog. +- You have the correct labels. + To be merged, PRs have to: -1. Pass all the CircleCI tests. +1. Pass all the CircleCI tests, and possibly all the codecov checks. 2. Have the necessary amount of approving reviews, even if you’re a long time contributor. You can ask one (or more) contributor to do that review, if you think they align more with the content of your PR. You need **one** review for documentation, tests, and small changes, and **two** reviews for bugs, refactoring and enhancements. -3. Contain at least a unit test for your contribution, if the PR contains code (it would be better if it contains an integration or function test and all the breaking tests necessary). If you’re not confident about writing tests, it is possible to refer to an issue that asks for the test to be written, or another (Draft) PR that contains the tests required. +3. Have at least a release-related label (or a `Skip release` label). +4. Have a short title that clearly explains in one sentence the aim of the PR. +5. Contain at least a unit test for your contribution, if the PR contains code (it would be better if it contains an integration or function test and all the breaking tests necessary). If you’re not confident about writing tests, it is possible to refer to an issue that asks for the test to be written, or another (Draft) PR that contains the tests required. + +As we’re trying to maintain at least a 90% code coverage, you’re strongly encouraged to write all the necessary tests not to drop below the threshold. If our coverage becomes too low, you might be asked to add more tests and/or your PR might be rejected. See the `Automatic Testing <#testing>`_ section. + +Don't merge your own pull request! That's a task for the main reviewer of your PR or the project manager. Remember that the project manager doesn't have to be a reviewer of your PR! + +.. _reviewing: + +Reviewing PRs +------------- +Reviewing PRs is an extremely important task in collaborative development. In fact, it is probably the task that requires the most time in the development, and it can be stressful for both the reviewer and the author. Remember that, as a PR Reviewer, you are granting that the codes works and integrates well with the rest of the repository, hence **you are responsible for the quality of the code and its next version release**. If it doesn't integrate well, later PR reviewers might have to ask for broader changes than expected. +Due to its importance, there are some good rules of thumbs that we need to follow while reviewing PRs: + +- Be respectful to the PR authors and be clear in what you are asking/suggesting - remember that, like you, they are contributing their spare time and doing their best job! +- If there is a Draft PR, you can comment on its development in the message board or making "Comment" reviews. Don't ask for changes, and especially, **don't approve the PR** +- If the PR graduated from Draft to full PR, check that it follows the sections `Pull requests <#pr>`_ and `Style Guide <#styling>`_ of these guidelines. If not, invite the author to do so before starting a review. +- **Don't limit your review to the parts that are changed**. Look at the entire file, see if the changes fit well in it, and see if the changes are properly addressed everywhere in the code - in the documentation, in the tests, and in other functions. Sometimes the differences reported don't show the full impact of the PR in the repository! +- Unless it's for typo fixes or similar, invite the author of the PR to make changes before actually doing them yourself. Request changes via comments or in the message board or by checking out the PR locally, making changes and then submitting a PR to the author's branch. +- If you're reviewing documentation, build it locally with `sphinx`. +- If you're asking for changes, **don't approve the PR**. Approve it only after everything was sufficiently addressed. Someone else might merge the PR in taking your word for granted. +- If you are the main reviewer, and the last reviewer required to approve the PR, merge the PR! + +Before approving and/or merging PRs, be sure that: + +- All the tests in Travis CI pass without errors. +- Prefereably, codecov checks pass as well - if they don't, pin the project manager. +- The title describes the content of the PR clearly enough to be meaningful on its own - remember that it will appear in the version changelog! +- The PR has the appropriate labels to trigger the appropriate version release and update the contributors table. + +.. _mainreviewer: + +Main reviewer +~~~~~~~~~~~~~ +At ``physiopy`` we use the "Assignees" section of a PR to mark the **main reviewer** for that PR. +The main reviewer: + +- Takes care of the reviewing process of the PR, in particular: + - Invites the reviewers to finish their review in a relatively short time. + - Checks that this document was respected in all its part, especially the part about `Pull Requests <#pr>`_. -As we’re trying to maintain at least a 90% code coverance, you’re strongly encouraged to write all the necessary tests not to drop below the threshold. If our coverance becomes too low, you might be asked to add more tests and/or your PR might be rejected. +- Is the one that is going to merge the PR. +- After the PR got merged and a new release was triggered, checks that: + - The documentation was updated correctly (if changed). + - The Pipy version of the repository coincides with the new release (if changed). + - New contributors or forms of contributions were correctly added in the README (if changed). .. _styling: @@ -192,13 +276,14 @@ Style Guide Docstrings should follow `numpydoc `_ convention. We encourage extensive documentation. The python code itself should follow `PEP8 `_ convention whenever possible: there are continuous integration tests checking that! You can use linters to help you write your code following style conventions. Linters are add-ons that you can run on the written script file. We suggest the use of **flake8** for Python 3. Many editors (Atoms, VScode, Sublimetext, ...) support addons for online lintering, which means you’ll see warnings and errors while you write the code - check out if your does! +Since we adopt `auto `_, the PR title will be automatically reported as part of the changelog when updating versions. Try to describe in one (short) sentence what your PR is about - possibly using the imperative and starting with a capital letter. For instance, a good PR title could be: ``Implement support for files`` or ``Reorder dictionary entries``, rather than `` support`` or ``reorders keys``. .. _testing: Automatic Testing ----------------- -physiopy uses Continuous Integration (CI) to make life easier. In particular, we use the `CircleCI `_ platform to run automatic testing! -**Automatic tests** are cold, robotic, emotionless, and opinionless tests that check that the program is doing what it is expected to. They are written by the developers and run (by CircleCI) every time they send a Pull Request to physiopy repositories. They complement the warm, human, emotional and opinionated **user tests**, as they tell us if a piece of code is failing. +``physiopy`` uses Continuous Integration (CI) to make life easier. In particular, we use the `CircleCI `_ platform to run automatic testing! +**Automatic tests** are cold, robotic, emotionless, and opinionless tests that check that the program is doing what it is expected to. They are written by the developers and run (by CircleCI) every time they send a Pull Request to ``physiopy`` repositories. They complement the warm, human, emotional and opinionated **user tests**, as they tell us if a piece of code is failing. CircleCI uses `pytest `_ to run the tests. The great thing about it is that you can run it in advance on your local version of the code! We can measure the amount of code that is tested with [codecov]8https://docs.pytest.org/en/latest/), which is an indication of how reliable our packages are! We try to maintain a 90% code coverage, and for this reason, PR should contain tests! The four main type of tests we use are: @@ -220,4 +305,4 @@ We welcome and recognize `all contributions `_ project.* +*— Based on contributing guidelines from the `STEMMRoleModels `_ project.* From 71858b9478d9b23980f8b641a2eeb732acbbf17c Mon Sep 17 00:00:00 2001 From: smoia Date: Wed, 14 Oct 2020 10:05:52 +0200 Subject: [PATCH 02/28] Add link to a reviewing best practice --- docs/contributorfile.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index 127859c75..a5066587c 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -234,7 +234,7 @@ Don't merge your own pull request! That's a task for the main reviewer of your P Reviewing PRs ------------- Reviewing PRs is an extremely important task in collaborative development. In fact, it is probably the task that requires the most time in the development, and it can be stressful for both the reviewer and the author. Remember that, as a PR Reviewer, you are granting that the codes works and integrates well with the rest of the repository, hence **you are responsible for the quality of the code and its next version release**. If it doesn't integrate well, later PR reviewers might have to ask for broader changes than expected. -Due to its importance, there are some good rules of thumbs that we need to follow while reviewing PRs: +There are many best practices to review code online, for instance `this one `_, but here are some good rules of thumbs that we need to follow while reviewing PRs: - Be respectful to the PR authors and be clear in what you are asking/suggesting - remember that, like you, they are contributing their spare time and doing their best job! - If there is a Draft PR, you can comment on its development in the message board or making "Comment" reviews. Don't ask for changes, and especially, **don't approve the PR** From e5339934246dd527f262b10a83638a2aef6f5d19 Mon Sep 17 00:00:00 2001 From: smoia Date: Wed, 14 Oct 2020 10:43:49 +0200 Subject: [PATCH 03/28] Add checklist and change type to PR templates --- .github/PULL_REQUEST_TEMPLATE.md | 33 ++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 7e2525842..8b43cb1c4 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,7 +1,36 @@ + + + Closes # -## Proposed Changes + + +## Proposed Changes + - - - - \ No newline at end of file + - + +## Change Type + +- [ ] `bugfix` (+0.0.1) +- [ ] `minor` (+0.1.0) +- [ ] `major` (+1.0.0) +- [ ] `internal` (no version update) +- [ ] `documentation` (no version update) + +## Checklist before review + + +- [ ] I added everything I wanted to add to this PR. +- [ ] \[Code or tests only\] I wrote/updated the necessary docstrings. +- [ ] \[Code or tests only\] I ran and passed tests locally. +- [ ] \[Documentation only\] I built the docs locally. +- [ ] My contribution is harmonious with the rest of the code: I'm not introducing repetitions and I respect the general style. + +- [ ] My code respects the adopted style, especially lintering conventions. +- [ ] The title of this PR is explicative on its own, enough to be understood as part of a changelog. +- [ ] I added or indicated the right labels. + +- [ ] Please, do review my PR while it's a draft and give me feedback! \ No newline at end of file From 41d946c9c9eb554a2c1d3b5ba801b242b2af9555 Mon Sep 17 00:00:00 2001 From: smoia Date: Wed, 14 Oct 2020 11:50:21 +0200 Subject: [PATCH 04/28] Reword --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 8b43cb1c4..8aa4a71e9 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -33,4 +33,4 @@ Closes # - [ ] The title of this PR is explicative on its own, enough to be understood as part of a changelog. - [ ] I added or indicated the right labels. -- [ ] Please, do review my PR while it's a draft and give me feedback! \ No newline at end of file +- [ ] Please, comment my PR while it's a draft and give me feedback on the development! \ No newline at end of file From 9e8f3a8a03bc1d166ab1a283d15071ea5324f932 Mon Sep 17 00:00:00 2001 From: smoia Date: Wed, 14 Oct 2020 17:39:55 +0200 Subject: [PATCH 05/28] Reword and fix typos --- docs/contributorfile.rst | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index a5066587c..a4e143576 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -188,7 +188,7 @@ Contribution workflow There are many descriptions of a good contribution workflow out there. For instance, we suggest to have a look at `tedana's workflow `_. At ``physiopy``, we follow a very similar workflow. The only three differences are: -- If you see an open issue that you would like to work on, check if it is assigned. If it is, ask the assignee if they need help before starting to work on it. +- If you see an open issue that you would like to work on, check if it is assigned. If it is, ask the assignee if they need help or want to be substituted before starting to work on it. - We ask you to test the code locally before merging it, and then, if possible, write some automatic tests for the code to be run in our Continuous Integration! Check the testing section below to know more. - We suggest opening a draft PR as soon as you can - so it’s easier for us to help you! @@ -233,7 +233,7 @@ Don't merge your own pull request! That's a task for the main reviewer of your P Reviewing PRs ------------- -Reviewing PRs is an extremely important task in collaborative development. In fact, it is probably the task that requires the most time in the development, and it can be stressful for both the reviewer and the author. Remember that, as a PR Reviewer, you are granting that the codes works and integrates well with the rest of the repository, hence **you are responsible for the quality of the code and its next version release**. If it doesn't integrate well, later PR reviewers might have to ask for broader changes than expected. +Reviewing PRs is an extremely important task in collaborative development. In fact, it is probably the task that requires the most time in the development, and it can be stressful for both the reviewer and the author. Remember that, as a PR Reviewer, you are granting that the codes works and integrates well with the rest of the repository, hence **you are responsible for the quality of the repository and its next version release**. If it doesn't integrate well, later PR reviewers might have to ask for broader changes than expected. There are many best practices to review code online, for instance `this one `_, but here are some good rules of thumbs that we need to follow while reviewing PRs: - Be respectful to the PR authors and be clear in what you are asking/suggesting - remember that, like you, they are contributing their spare time and doing their best job! @@ -241,14 +241,14 @@ There are many best practices to review code online, for instance `this one `_ and `Style Guide <#styling>`_ of these guidelines. If not, invite the author to do so before starting a review. - **Don't limit your review to the parts that are changed**. Look at the entire file, see if the changes fit well in it, and see if the changes are properly addressed everywhere in the code - in the documentation, in the tests, and in other functions. Sometimes the differences reported don't show the full impact of the PR in the repository! - Unless it's for typo fixes or similar, invite the author of the PR to make changes before actually doing them yourself. Request changes via comments or in the message board or by checking out the PR locally, making changes and then submitting a PR to the author's branch. -- If you're reviewing documentation, build it locally with `sphinx`. +- If you're reviewing documentation, build it locally with ``sphinx``. - If you're asking for changes, **don't approve the PR**. Approve it only after everything was sufficiently addressed. Someone else might merge the PR in taking your word for granted. - If you are the main reviewer, and the last reviewer required to approve the PR, merge the PR! Before approving and/or merging PRs, be sure that: -- All the tests in Travis CI pass without errors. -- Prefereably, codecov checks pass as well - if they don't, pin the project manager. +- All the tests in CircleCI/Azure pass without errors. +- Prefereably, codecov checks pass as well. If they don't, discuss what to do. - The title describes the content of the PR clearly enough to be meaningful on its own - remember that it will appear in the version changelog! - The PR has the appropriate labels to trigger the appropriate version release and update the contributors table. @@ -256,17 +256,23 @@ Before approving and/or merging PRs, be sure that: Main reviewer ~~~~~~~~~~~~~ -At ``physiopy`` we use the "Assignees" section of a PR to mark the **main reviewer** for that PR. +At ``physiopy`` we use the "Assignees" section of a PR to mark the **main reviewer** for that PR. The main reviewer is the main responsible **for the quality of the repository and its next version release**, as well as **for the behaviour of the other reviewers**. The main reviewer: - Takes care of the reviewing process of the PR, in particular: - Invites the reviewers to finish their review in a relatively short time. - Checks that this document was respected in all its part, especially the part about `Pull Requests <#pr>`_. - -- Is the one that is going to merge the PR. + - Invites other Reviewers to respect this document, especially the part about `reviews <#reviewing>`_, helps them doing it, and checks that they do. + - If a Reviewer keeps not respecting this document, flags them to the project manager. +- Decides what to do in case of a coverage decrease (in *codecov/patch*). +- In case of missing tests or updates to user documentation: + - Asks for more documentation or tests before approving the PR, *or* + - Checks that the adequate issues have been opened to address the lack of documentation or tests (1 issue per item). +- Double-checks that the title is clear and the labels are correct to trigger an adequate ``auto`` release - feel free to change them. +- **Is the one that is going to merge the PR.** - After the PR got merged and a new release was triggered, checks that: - The documentation was updated correctly (if changed). - - The Pipy version of the repository coincides with the new release (if changed). + - The Pypi version of the repository coincides with the new release (if changed). - New contributors or forms of contributions were correctly added in the README (if changed). .. _styling: @@ -275,7 +281,8 @@ Style Guide ----------- Docstrings should follow `numpydoc `_ convention. We encourage extensive documentation. The python code itself should follow `PEP8 `_ convention whenever possible: there are continuous integration tests checking that! -You can use linters to help you write your code following style conventions. Linters are add-ons that you can run on the written script file. We suggest the use of **flake8** for Python 3. Many editors (Atoms, VScode, Sublimetext, ...) support addons for online lintering, which means you’ll see warnings and errors while you write the code - check out if your does! +You can use linters to help you write your code following style conventions. Linters are add-ons that you can run on the written script file. We suggest the use of **flake8** for Python 3. Many editors (Atom, VScode, Sublimetext, ...) support addons for online lintering, which means you’ll see warnings and errors while you write the code - check out if your does! + Since we adopt `auto `_, the PR title will be automatically reported as part of the changelog when updating versions. Try to describe in one (short) sentence what your PR is about - possibly using the imperative and starting with a capital letter. For instance, a good PR title could be: ``Implement support for files`` or ``Reorder dictionary entries``, rather than `` support`` or ``reorders keys``. .. _testing: @@ -285,7 +292,7 @@ Automatic Testing ``physiopy`` uses Continuous Integration (CI) to make life easier. In particular, we use the `CircleCI `_ platform to run automatic testing! **Automatic tests** are cold, robotic, emotionless, and opinionless tests that check that the program is doing what it is expected to. They are written by the developers and run (by CircleCI) every time they send a Pull Request to ``physiopy`` repositories. They complement the warm, human, emotional and opinionated **user tests**, as they tell us if a piece of code is failing. CircleCI uses `pytest `_ to run the tests. The great thing about it is that you can run it in advance on your local version of the code! -We can measure the amount of code that is tested with [codecov]8https://docs.pytest.org/en/latest/), which is an indication of how reliable our packages are! We try to maintain a 90% code coverage, and for this reason, PR should contain tests! +We can measure the amount of code that is tested with `codecov `_, which is an indication of how reliable our packages are! We try to maintain a 90% code coverage, and for this reason, PR should contain tests! The four main type of tests we use are: 1. Unit tests From d684e7e5ecc5a7f21570a93ab1a2275b1b8e27fc Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Thu, 15 Oct 2020 17:56:00 +0200 Subject: [PATCH 06/28] Update .github/PULL_REQUEST_TEMPLATE.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Eneko Uruñuela <13706448+eurunuela@users.noreply.github.com> --- .github/PULL_REQUEST_TEMPLATE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 8aa4a71e9..a6b51ce85 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,6 +1,6 @@ - + Closes # @@ -33,4 +33,4 @@ Closes # - [ ] The title of this PR is explicative on its own, enough to be understood as part of a changelog. - [ ] I added or indicated the right labels. -- [ ] Please, comment my PR while it's a draft and give me feedback on the development! \ No newline at end of file +- [ ] Please, comment my PR while it's a draft and give me feedback on the development! From a635d87cdf7ef12080d6f415fe6f77d4d63390f3 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Thu, 15 Oct 2020 17:56:11 +0200 Subject: [PATCH 07/28] Update .github/PULL_REQUEST_TEMPLATE.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Eneko Uruñuela <13706448+eurunuela@users.noreply.github.com> --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index a6b51ce85..df94decfc 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -3,7 +3,7 @@ Closes # - + ## Proposed Changes From 3b33c3606064b6dbcb0a392be14bce77e9f4a3d2 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Thu, 15 Oct 2020 17:56:22 +0200 Subject: [PATCH 08/28] Update .github/PULL_REQUEST_TEMPLATE.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Eneko Uruñuela <13706448+eurunuela@users.noreply.github.com> --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index df94decfc..687efc972 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -21,7 +21,7 @@ Closes # - [ ] `documentation` (no version update) ## Checklist before review - + - [ ] I added everything I wanted to add to this PR. - [ ] \[Code or tests only\] I wrote/updated the necessary docstrings. From 618d40b0fe9dbbe452b55ebb13b094f1b3691365 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Thu, 15 Oct 2020 17:56:38 +0200 Subject: [PATCH 09/28] Update docs/contributing.rst MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Eneko Uruñuela <13706448+eurunuela@users.noreply.github.com> --- docs/contributing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributing.rst b/docs/contributing.rst index b79560961..8b072be75 100644 --- a/docs/contributing.rst +++ b/docs/contributing.rst @@ -13,7 +13,7 @@ Follow these steps to get started: 1. Have a look at the `contributor guide `_ page as well as the `code of conduct `_. 2. Make sure that you have a GitHub account. You can set up a `free GitHub account `_; here are some `instructions `_. -3. If you intend to contribute code and/or use the ``physiopy`` packages in any way, check that you have ``git`` and ``pip`` installed on your system. Then install ``phys2bids`` as a developer. This will let you run the program with the latest modification, without requiring to re-install it every time. +3. If you intend to contribute code and/or use the ``physiopy`` packages in any way, check that you have ``git`` and ``pip`` installed on your system. Then install ``phys2bids`` as a developer. This will let you run the program with the latest modifications, without requiring to re-install it every time. .. _`all-contributors`: https://github.com/all-contributors/all-contributors From 9eb7f9d987822863f731cea820efdbb3eb4fa339 Mon Sep 17 00:00:00 2001 From: Rachael Stickland Date: Mon, 19 Oct 2020 11:50:21 -0500 Subject: [PATCH 10/28] doc_update --- .github/PULL_REQUEST_TEMPLATE.md | 4 +++- docs/contributorfile.rst | 10 +++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 687efc972..faea51fc4 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -30,7 +30,9 @@ Closes # - [ ] My contribution is harmonious with the rest of the code: I'm not introducing repetitions and I respect the general style. - [ ] My code respects the adopted style, especially lintering conventions. -- [ ] The title of this PR is explicative on its own, enough to be understood as part of a changelog. +- [ ] The title of this PR is explanatory on its own, enough to be understood as part of a changelog. - [ ] I added or indicated the right labels. + +- [ ] I added information regarding the timeline of completion for this PR. - [ ] Please, comment my PR while it's a draft and give me feedback on the development! diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index a4e143576..5eea8fdca 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -98,7 +98,7 @@ We use `readthedocs `_ to create our documentation. Ev Contributing code through GitHub ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This section covers 90% of the contributions a project like ``physiopy`` receives - code, documentation and tests. -The best way to make this kind of contributions, in a nutshell, is to: +The best way to make this kind of contribution, in a nutshell, is to: 1. Open an issue with the intended modifications. 2. Label it, discuss it, (self-)assign it. 3. Open a Pull Request (PR) to resolve the issue and label it. @@ -126,7 +126,7 @@ At ``physiopy``, we use Issues and Milestones to keep track of and organise our Labels ------ The current list of labels are `here `_. They can be used for **Issues**, **PRs**, or both. -We use `auto `_ to automatise our semantic versioning and Pypi upload, so **it's extremely important to use the right PR labels**! +We use `auto `_ to automate our semantic versioning and Pypi upload, so **it's extremely important to use the right PR labels**! .. _issueprlabels: @@ -145,7 +145,7 @@ Issue & PR labels Issue-only labels ~~~~~~~~~~~~~~~~~~ - Bug: Something isn’t working. It either breaks the code or has an unexpected outcome. -- Community: This issue contains information about the `physiopy` community (e.g. the next developer call) +- Community: This issue contains information about the ``physiopy`` community (e.g. the next developer call) - Discussion: Discussion of a concept or implementation. These Issues are prone to be open ad infinitum. Jump in the conversation if you want! - Good first issue: Good for newcomers. These issues calls for a **fairly** easy enhancement, or for a change that helps/requires getting to know the code better. They have educational value, and for this reason, unless urgent, experts in the topic should refrain from closing them - but help newcomers closing them. - Hacktoberfest: Dedicated to the hacktoberfest event, so that people can help and feel good about it (and show it with a T-shirt!). **Such commits will not be recognised in the all-contributor table, unless otherwise specified**. @@ -262,13 +262,13 @@ The main reviewer: - Takes care of the reviewing process of the PR, in particular: - Invites the reviewers to finish their review in a relatively short time. - Checks that this document was respected in all its part, especially the part about `Pull Requests <#pr>`_. - - Invites other Reviewers to respect this document, especially the part about `reviews <#reviewing>`_, helps them doing it, and checks that they do. + - Invites other Reviewers to respect this document, especially the part about `reviews <#reviewing>`_, helps them in doing so, and checks that they do. - If a Reviewer keeps not respecting this document, flags them to the project manager. - Decides what to do in case of a coverage decrease (in *codecov/patch*). - In case of missing tests or updates to user documentation: - Asks for more documentation or tests before approving the PR, *or* - Checks that the adequate issues have been opened to address the lack of documentation or tests (1 issue per item). -- Double-checks that the title is clear and the labels are correct to trigger an adequate ``auto`` release - feel free to change them. +- Double-checks that the title is clear and the labels are correct to trigger an adequate ``auto`` release - feel free to change them. - **Is the one that is going to merge the PR.** - After the PR got merged and a new release was triggered, checks that: - The documentation was updated correctly (if changed). From 42056e2fdf9e37806e664a19d877dfe2c7474f88 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Tue, 20 Oct 2020 21:43:42 +0200 Subject: [PATCH 11/28] Update .github/PULL_REQUEST_TEMPLATE.md Co-authored-by: Taylor Salo --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 687efc972..53be12db5 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -29,7 +29,7 @@ Closes # - [ ] \[Documentation only\] I built the docs locally. - [ ] My contribution is harmonious with the rest of the code: I'm not introducing repetitions and I respect the general style. -- [ ] My code respects the adopted style, especially lintering conventions. +- [ ] My code respects the adopted style, especially linting conventions. - [ ] The title of this PR is explicative on its own, enough to be understood as part of a changelog. - [ ] I added or indicated the right labels. From b4401d30f9a72689f4d11f86f48dfe8ea6cde529 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Tue, 20 Oct 2020 21:43:52 +0200 Subject: [PATCH 12/28] Update .github/PULL_REQUEST_TEMPLATE.md Co-authored-by: Taylor Salo --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 53be12db5..7500104bc 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -33,4 +33,4 @@ Closes # - [ ] The title of this PR is explicative on its own, enough to be understood as part of a changelog. - [ ] I added or indicated the right labels. -- [ ] Please, comment my PR while it's a draft and give me feedback on the development! +- [ ] Please, comment on my PR while it's a draft and give me feedback on the development! From 78da4865049efe5908561037c1c956ce91f08e24 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Tue, 20 Oct 2020 21:44:15 +0200 Subject: [PATCH 13/28] Update docs/contributorfile.rst Co-authored-by: Taylor Salo --- docs/contributorfile.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index a4e143576..f9d643902 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -206,7 +206,7 @@ However, if it’s an incomplete PR, please open a **Draft PR**. That helps us p Reviewing PRs is a time consuming task, that can be stressful for both the reviewer and the author. Avoiding wasting time and the need of little fixes - such as fixing grammar mistakes and typos, styling code, or adopting conventions - is a good start for a successful (and quick) review. Before graduating a Draft PR to a PR ready for review, please check that: - You did all you wanted to include in your PR. If at a later stage you realise something is missing and it's not a minor thing, you will need to open a new PR. -- If your contribution contains code or tests, you run and passed all the tests locally with `pytest`. +- If your contribution contains code or tests, you ran and passed all of the tests locally with `pytest`. - If you're writing documentation, you built it locally with `sphinx` and the format is what you intended. - Your code is harmonious with the rest of the code - no repetitions of any sort! - Your code respects the `adopted Style <#styling>`_, especially: From b22a5166d9025d0578c830849cbdc845d8e976ec Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Tue, 20 Oct 2020 21:45:13 +0200 Subject: [PATCH 14/28] Update .github/PULL_REQUEST_TEMPLATE.md --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 7500104bc..d6c15dc0c 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -27,7 +27,7 @@ Closes # - [ ] \[Code or tests only\] I wrote/updated the necessary docstrings. - [ ] \[Code or tests only\] I ran and passed tests locally. - [ ] \[Documentation only\] I built the docs locally. -- [ ] My contribution is harmonious with the rest of the code: I'm not introducing repetitions and I respect the general style. +- [ ] My contribution is harmonious with the rest of the code: I'm not introducing repetitions. - [ ] My code respects the adopted style, especially linting conventions. - [ ] The title of this PR is explicative on its own, enough to be understood as part of a changelog. From a82a285e8b2a74c4a69f234c221f07cf30d28631 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Tue, 20 Oct 2020 21:45:40 +0200 Subject: [PATCH 15/28] Update .github/PULL_REQUEST_TEMPLATE.md --- .github/PULL_REQUEST_TEMPLATE.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index d6c15dc0c..07933ffab 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -28,7 +28,6 @@ Closes # - [ ] \[Code or tests only\] I ran and passed tests locally. - [ ] \[Documentation only\] I built the docs locally. - [ ] My contribution is harmonious with the rest of the code: I'm not introducing repetitions. - - [ ] My code respects the adopted style, especially linting conventions. - [ ] The title of this PR is explicative on its own, enough to be understood as part of a changelog. - [ ] I added or indicated the right labels. From 12aa4b74583d7e336bc012d9b7cd6f0a845ffb68 Mon Sep 17 00:00:00 2001 From: smoia Date: Tue, 20 Oct 2020 22:48:13 +0200 Subject: [PATCH 16/28] Add sentence about narrowing PR scope --- docs/contributorfile.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index c7d0966a6..617a857f3 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -197,6 +197,7 @@ At ``physiopy``, we follow a very similar workflow. The only three differences a Pull Requests ------------- To improve understanding pull requests "at a glance", we use the labels listed above. Multiple labels can be assigned, but it's a good idea to keep different types of contributions separated, unless they are minimal - for instance, you might want to open a PR for code, one for documentation and one for testing. +We also strongly advice to keep your narrow the changes you're introducing with your PR to your real aim. Little corrections here and there in the code that you're already modifying are a great help, but they risk to complicate the nature of the PR and the reviewing process. It is much better to open another PR with the objective of doing such corrections! In general, if you're tempted to assign more than one label that would trigger a release, you might want to split your PR instead. When opening a pull request, assign it at least one label. From 39e40a8944c8e8cdfd0633a2e2134d69b8fd7969 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Wed, 21 Oct 2020 22:54:03 +0200 Subject: [PATCH 17/28] Update .github/PULL_REQUEST_TEMPLATE.md Co-authored-by: Taylor Salo --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index d40600975..c91fa8711 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,4 +1,4 @@ - + Closes # From 4b6e7f0ab5230cb2045dbac26ea82d06c63b3a60 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Wed, 21 Oct 2020 22:54:15 +0200 Subject: [PATCH 18/28] Update .github/PULL_REQUEST_TEMPLATE.md Co-authored-by: Taylor Salo --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index c91fa8711..91e7894ca 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -13,7 +13,7 @@ Closes # - ## Change Type - + - [ ] `bugfix` (+0.0.1) - [ ] `minor` (+0.1.0) - [ ] `major` (+1.0.0) From f9ca41f767aeecb1fa22157e5619ffabe1d96d3a Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Wed, 21 Oct 2020 22:54:27 +0200 Subject: [PATCH 19/28] Update .github/PULL_REQUEST_TEMPLATE.md Co-authored-by: Taylor Salo --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 91e7894ca..420cd189a 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -22,7 +22,7 @@ Closes # ## Checklist before review - + - [ ] I added everything I wanted to add to this PR. - [ ] \[Code or tests only\] I wrote/updated the necessary docstrings. - [ ] \[Code or tests only\] I ran and passed tests locally. From 05f9aadbd93a6704111c67e0bbf3d57eb14e22a8 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Wed, 21 Oct 2020 22:54:52 +0200 Subject: [PATCH 20/28] Update docs/contributorfile.rst Co-authored-by: Taylor Salo --- docs/contributorfile.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index 617a857f3..785981c3b 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -49,7 +49,7 @@ Our main goal is to help collect, analyse and share physiological data, interfac Joining the conversation ------------------------ -We’re trying to keep all the conversation related to the project development in GitHub `issues `_. +We’re trying to keep all conversation related to project development in GitHub `issues `_. We maintain a `gitter chat room `_ for more informal conversations and general project updates. We also have a dev call once a month - specifically the second Thursday of the month! If you want to participate, drop a line in gitter! When interacting in the common channels, please adhere to our `code of conduct `_. From d20a0644ae6ef60f121b7c27a8abb0ba3898e1db Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Wed, 21 Oct 2020 22:55:55 +0200 Subject: [PATCH 21/28] Update docs/contributorfile.rst Co-authored-by: Taylor Salo --- docs/contributorfile.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index 785981c3b..ee948a8aa 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -197,7 +197,7 @@ At ``physiopy``, we follow a very similar workflow. The only three differences a Pull Requests ------------- To improve understanding pull requests "at a glance", we use the labels listed above. Multiple labels can be assigned, but it's a good idea to keep different types of contributions separated, unless they are minimal - for instance, you might want to open a PR for code, one for documentation and one for testing. -We also strongly advice to keep your narrow the changes you're introducing with your PR to your real aim. Little corrections here and there in the code that you're already modifying are a great help, but they risk to complicate the nature of the PR and the reviewing process. It is much better to open another PR with the objective of doing such corrections! +We also strongly advise to keep the changes you're introducing with your PR limited to your original goal. Little corrections here and there in the code that you're already modifying are a great help, but they risk complicating the nature of the PR and the reviewing process. It is much better to open another PR with the objective of doing such corrections! In general, if you're tempted to assign more than one label that would trigger a release, you might want to split your PR instead. When opening a pull request, assign it at least one label. From 01d67a40644e80c2da9a88263308ba0654c0dde9 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Wed, 21 Oct 2020 23:00:21 +0200 Subject: [PATCH 22/28] Update docs/contributorfile.rst Co-authored-by: Taylor Salo --- docs/contributorfile.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index ee948a8aa..c6fd54052 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -269,7 +269,7 @@ The main reviewer: - In case of missing tests or updates to user documentation: - Asks for more documentation or tests before approving the PR, *or* - Checks that the adequate issues have been opened to address the lack of documentation or tests (1 issue per item). -- Double-checks that the title is clear and the labels are correct to trigger an adequate ``auto`` release - feel free to change them. +- Double-checks that the title is clear and the labels are correct to trigger an appropriate ``auto`` release - feel free to change them. - **Is the one that is going to merge the PR.** - After the PR got merged and a new release was triggered, checks that: - The documentation was updated correctly (if changed). From d62402ad9741c415ce90c38708aa90f90d7c6d7c Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Wed, 21 Oct 2020 23:00:35 +0200 Subject: [PATCH 23/28] Update docs/contributorfile.rst Co-authored-by: Taylor Salo --- docs/contributorfile.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index c6fd54052..9f44e3ff1 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -271,7 +271,7 @@ The main reviewer: - Checks that the adequate issues have been opened to address the lack of documentation or tests (1 issue per item). - Double-checks that the title is clear and the labels are correct to trigger an appropriate ``auto`` release - feel free to change them. - **Is the one that is going to merge the PR.** -- After the PR got merged and a new release was triggered, checks that: +- After the PR has been merged and a new release has been triggered, checks that: - The documentation was updated correctly (if changed). - The Pypi version of the repository coincides with the new release (if changed). - New contributors or forms of contributions were correctly added in the README (if changed). From 528e04b9ce9a10c7e4b26dbff25c248a19645f23 Mon Sep 17 00:00:00 2001 From: Stefano Moia Date: Wed, 21 Oct 2020 23:00:50 +0200 Subject: [PATCH 24/28] Update docs/contributorfile.rst Co-authored-by: Taylor Salo --- docs/contributorfile.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index 9f44e3ff1..dbe25668b 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -268,7 +268,7 @@ The main reviewer: - Decides what to do in case of a coverage decrease (in *codecov/patch*). - In case of missing tests or updates to user documentation: - Asks for more documentation or tests before approving the PR, *or* - - Checks that the adequate issues have been opened to address the lack of documentation or tests (1 issue per item). + - Checks that the appropriate issues have been opened to address the lack of documentation or tests (1 issue per item). - Double-checks that the title is clear and the labels are correct to trigger an appropriate ``auto`` release - feel free to change them. - **Is the one that is going to merge the PR.** - After the PR has been merged and a new release has been triggered, checks that: From 00e504ccd356dc8795b64b231598e74a37ce1401 Mon Sep 17 00:00:00 2001 From: smoia Date: Tue, 10 Nov 2020 23:16:04 +0100 Subject: [PATCH 25/28] Update rules about suggestion tool and direct PR --- docs/contributorfile.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index dbe25668b..a91b3fade 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -241,7 +241,8 @@ There are many best practices to review code online, for instance `this one `_ and `Style Guide <#styling>`_ of these guidelines. If not, invite the author to do so before starting a review. - **Don't limit your review to the parts that are changed**. Look at the entire file, see if the changes fit well in it, and see if the changes are properly addressed everywhere in the code - in the documentation, in the tests, and in other functions. Sometimes the differences reported don't show the full impact of the PR in the repository! -- Unless it's for typo fixes or similar, invite the author of the PR to make changes before actually doing them yourself. Request changes via comments or in the message board or by checking out the PR locally, making changes and then submitting a PR to the author's branch. +- If your want to make Pull Requests an educational process, invite the author of the PR to make changes before actually doing them yourself. Request changes via comments or in the message board or by checking out the PR locally, making changes and then submitting a PR to the author's branch. +- If you decide to use the suggestion tool in reviews, or to start a PR to the branch in review, please alert the Project Manager. Bots might automatically assign you contribution types that will have to be removed (remember, your contribution in this case is "Reviewer"). - If you're reviewing documentation, build it locally with ``sphinx``. - If you're asking for changes, **don't approve the PR**. Approve it only after everything was sufficiently addressed. Someone else might merge the PR in taking your word for granted. - If you are the main reviewer, and the last reviewer required to approve the PR, merge the PR! From 467fe6221779a5ee52aab5e33b7c28cdcdccef91 Mon Sep 17 00:00:00 2001 From: smoia Date: Tue, 10 Nov 2020 23:22:39 +0100 Subject: [PATCH 26/28] Update labels in PR rules --- docs/contributorfile.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index a91b3fade..1f6f55ad1 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -196,8 +196,8 @@ At ``physiopy``, we follow a very similar workflow. The only three differences a Pull Requests ------------- -To improve understanding pull requests "at a glance", we use the labels listed above. Multiple labels can be assigned, but it's a good idea to keep different types of contributions separated, unless they are minimal - for instance, you might want to open a PR for code, one for documentation and one for testing. -We also strongly advise to keep the changes you're introducing with your PR limited to your original goal. Little corrections here and there in the code that you're already modifying are a great help, but they risk complicating the nature of the PR and the reviewing process. It is much better to open another PR with the objective of doing such corrections! +To improve understanding pull requests "at a glance" and use the power of ``auto``, we use the labels listed above. Multiple labels can be assigned to a PR - in fact, all those that you think are relevant. +We strongly advise to keep the changes you're introducing with your PR limited to your original goal. Little corrections here and there in the code that you're already modifying are a great help, but they risk complicating the nature of the PR and the reviewing process. It is much better to open another PR with the objective of doing such corrections! In general, if you're tempted to assign more than one label that would trigger a release, you might want to split your PR instead. When opening a pull request, assign it at least one label. @@ -242,7 +242,7 @@ There are many best practices to review code online, for instance `this one `_ and `Style Guide <#styling>`_ of these guidelines. If not, invite the author to do so before starting a review. - **Don't limit your review to the parts that are changed**. Look at the entire file, see if the changes fit well in it, and see if the changes are properly addressed everywhere in the code - in the documentation, in the tests, and in other functions. Sometimes the differences reported don't show the full impact of the PR in the repository! - If your want to make Pull Requests an educational process, invite the author of the PR to make changes before actually doing them yourself. Request changes via comments or in the message board or by checking out the PR locally, making changes and then submitting a PR to the author's branch. -- If you decide to use the suggestion tool in reviews, or to start a PR to the branch in review, please alert the Project Manager. Bots might automatically assign you contribution types that will have to be removed (remember, your contribution in this case is "Reviewer"). +- If you decide to use the suggestion tool in reviews, or to start a PR to the branch under review, please alert the Project Manager. Bots might automatically assign you contribution types that will have to be removed (remember, your contribution in this case is "Reviewer"). Instead of starting a PR to the branch under review, think about opening a new PR with those modifications (unless they are needed to pass tests), and alert the Main Reviewer. In any case **don't commit directly to the branch under review**! - If you're reviewing documentation, build it locally with ``sphinx``. - If you're asking for changes, **don't approve the PR**. Approve it only after everything was sufficiently addressed. Someone else might merge the PR in taking your word for granted. - If you are the main reviewer, and the last reviewer required to approve the PR, merge the PR! From 1919324cec0b6776bd3d8f621e5c106bf43e6d67 Mon Sep 17 00:00:00 2001 From: smoia Date: Wed, 11 Nov 2020 12:18:21 +0100 Subject: [PATCH 27/28] Language corrections and rewording of PR section for better clarity --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- docs/contributorfile.rst | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 420cd189a..b8a3ef828 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -7,7 +7,7 @@ Closes # ## Proposed Changes - + - - - diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index 1f6f55ad1..7128659b8 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -109,7 +109,7 @@ Issues and PR chats are great to maintain track of the conversation on the contr Contributing with Pull Requests Reviews ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -One big challenge in this kind of software development is merging code accurately but without waiting too much time. +A big challenge of software development is merging code accurately without having to wait too much time. For this reason, Reviewers for PRs are more than welcome! It is a task that requires some experience, but it's very necessary! Read the `related section below <#reviewing>`_ to start! @@ -197,8 +197,8 @@ At ``physiopy``, we follow a very similar workflow. The only three differences a Pull Requests ------------- To improve understanding pull requests "at a glance" and use the power of ``auto``, we use the labels listed above. Multiple labels can be assigned to a PR - in fact, all those that you think are relevant. -We strongly advise to keep the changes you're introducing with your PR limited to your original goal. Little corrections here and there in the code that you're already modifying are a great help, but they risk complicating the nature of the PR and the reviewing process. It is much better to open another PR with the objective of doing such corrections! -In general, if you're tempted to assign more than one label that would trigger a release, you might want to split your PR instead. +We strongly advise to keep the changes you're introducing with your PR limited to your original goal. Adding to the scope of your PR little style corrections or code refactoring here and there in the code that you're already modifying is a great help, but when they become too much (and they are not relevant to your PR) they risk complicating the nature of the PR and the reviewing process. It is much better to open another PR with the objective of doing such corrections! +Moreover, if you're tempted to assign more than one label that would trigger a release (e.g. `bug` and `minormod`, or `bug` and `majormod`, etc. etc.), you might want to split your PR instead. When opening a pull request, assign it at least one label. We encourage you to open a PR as soon as possible - even before you finish working on them. This is useful especially to you - so that you can receive comments and suggestions early on, rather than having to process a lot of comments in the final review step! From 216d8bf32cb8c71a46c3a38dcbdf29574c9817e6 Mon Sep 17 00:00:00 2001 From: smoia Date: Wed, 11 Nov 2020 15:40:05 +0100 Subject: [PATCH 28/28] Rewording --- docs/contributorfile.rst | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/contributorfile.rst b/docs/contributorfile.rst index 7128659b8..1b38d6f7b 100644 --- a/docs/contributorfile.rst +++ b/docs/contributorfile.rst @@ -204,7 +204,7 @@ When opening a pull request, assign it at least one label. We encourage you to open a PR as soon as possible - even before you finish working on them. This is useful especially to you - so that you can receive comments and suggestions early on, rather than having to process a lot of comments in the final review step! However, if it’s an incomplete PR, please open a **Draft PR**. That helps us process PRs by knowing which one to have a look first - and how picky to be when doing so. -Reviewing PRs is a time consuming task, that can be stressful for both the reviewer and the author. Avoiding wasting time and the need of little fixes - such as fixing grammar mistakes and typos, styling code, or adopting conventions - is a good start for a successful (and quick) review. Before graduating a Draft PR to a PR ready for review, please check that: +Reviewing PRs is a time consuming task and it can be stressful for both the reviewer and the author. Avoiding wasting time and the need of little fixes - such as fixing grammar mistakes and typos, styling code, or adopting conventions - is a good start for a successful (and quick) review. Before graduating a Draft PR to a PR ready for review, please check that: - You did all you wanted to include in your PR. If at a later stage you realise something is missing and it's not a minor thing, you will need to open a new PR. - If your contribution contains code or tests, you ran and passed all of the tests locally with `pytest`. @@ -213,7 +213,7 @@ Reviewing PRs is a time consuming task, that can be stressful for both the revie - Your code respects the `adopted Style <#styling>`_, especially: - Your code is lintered adequately and respects the `PEP8 `_ convention. - Your docstrings follow the `numpydoc `_ convention. - - There is no grammar mistake or typo and the text is fluid. + - There are no typos or grammatical mistakes and the text is fluid. - The code is sufficiently commented and the comments are clear. - Your PR title is clear enough to be meaningful when appended to the version changelog. - You have the correct labels. @@ -226,7 +226,7 @@ To be merged, PRs have to: 4. Have a short title that clearly explains in one sentence the aim of the PR. 5. Contain at least a unit test for your contribution, if the PR contains code (it would be better if it contains an integration or function test and all the breaking tests necessary). If you’re not confident about writing tests, it is possible to refer to an issue that asks for the test to be written, or another (Draft) PR that contains the tests required. -As we’re trying to maintain at least a 90% code coverage, you’re strongly encouraged to write all the necessary tests not to drop below the threshold. If our coverage becomes too low, you might be asked to add more tests and/or your PR might be rejected. See the `Automatic Testing <#testing>`_ section. +As we’re trying to maintain at least 90% code coverage, you’re strongly encouraged to write all of the tests necessary to keep coverage above that threshold. If coverage drops too low, you might be asked to add more tests and/or your PR might be rejected. See the `Automatic Testing <#testing>`_ section. Don't merge your own pull request! That's a task for the main reviewer of your PR or the project manager. Remember that the project manager doesn't have to be a reviewer of your PR! @@ -234,7 +234,10 @@ Don't merge your own pull request! That's a task for the main reviewer of your P Reviewing PRs ------------- -Reviewing PRs is an extremely important task in collaborative development. In fact, it is probably the task that requires the most time in the development, and it can be stressful for both the reviewer and the author. Remember that, as a PR Reviewer, you are granting that the codes works and integrates well with the rest of the repository, hence **you are responsible for the quality of the repository and its next version release**. If it doesn't integrate well, later PR reviewers might have to ask for broader changes than expected. +Reviewing PRs is an extremely important task in collaborative development. +In fact, it is probably the task that requires the most time in the development, and it can be stressful for both the reviewer and the author. +Remember that, as a PR Reviewer, you are guaranteeing that the changes work and integrate well with the rest of the repository, hence **you are responsible for the quality of the repository and its next version release**. +If they don't integrate well, later PR reviewers might have to ask for broader changes than expected. There are many best practices to review code online, for instance `this one `_, but here are some good rules of thumbs that we need to follow while reviewing PRs: - Be respectful to the PR authors and be clear in what you are asking/suggesting - remember that, like you, they are contributing their spare time and doing their best job! @@ -258,12 +261,12 @@ Before approving and/or merging PRs, be sure that: Main reviewer ~~~~~~~~~~~~~ -At ``physiopy`` we use the "Assignees" section of a PR to mark the **main reviewer** for that PR. The main reviewer is the main responsible **for the quality of the repository and its next version release**, as well as **for the behaviour of the other reviewers**. +At ``physiopy`` we use the "Assignees" section of a PR to mark the **main reviewer** for that PR. The main reviewer is the primary person responsible **for the quality of the repository and its next version release**, as well as **for the behaviour of the other reviewers**. The main reviewer: - Takes care of the reviewing process of the PR, in particular: - Invites the reviewers to finish their review in a relatively short time. - - Checks that this document was respected in all its part, especially the part about `Pull Requests <#pr>`_. + - Checks that all elements of this document were respected, especially the part about `Pull Requests <#pr>`_. - Invites other Reviewers to respect this document, especially the part about `reviews <#reviewing>`_, helps them in doing so, and checks that they do. - If a Reviewer keeps not respecting this document, flags them to the project manager. - Decides what to do in case of a coverage decrease (in *codecov/patch*).