-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DOC] Adds developer guideline start #446
Changes from 15 commits
111cd6b
7891407
36cf608
d1d945c
18a2e6b
ff70cac
e7e542e
ae88853
090f786
f5cc4d7
74b7462
079a41a
79e7b52
4b7f751
94799a7
6a3043f
db327ff
558b74d
30d125a
95921d4
a535ee9
d722317
a63ccfe
aa41ffe
8beeecd
aa621c9
a03f095
93c465b
9ad1eca
1c15264
d1af26b
415d272
302e17b
1d75896
5af5708
e4fe82f
886ce30
157bc7a
52ab11b
eecbf62
eecd33b
267bf66
6aa2296
b7374a1
3ad460c
13e3daa
6a6a71f
c720db1
7102ff0
6753c82
dbc6d6e
1bd9b19
51142ef
244f865
ee3ea7c
35f0a44
dd1791f
8eab7e1
88d176d
8c68d89
7491b92
0cc2369
cd4b709
28368a8
f9a7295
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,6 @@ Here are some [instructions][link_signupinstructions]. | |
Already know what you're looking for in this guide? Jump to the following sections: | ||
|
||
* [Joining the conversation](#joining-the-conversation) | ||
* [Monthly developer calls](#monthly-developer-calls) | ||
* [Contributing small documentation changes](#contributing-small-documentation-changes) | ||
* [Contributing through Github](#contributing-through-github) | ||
* [Understanding issues, milestones, and project boards](#understanding-issues-milestones-and-project-boards) | ||
|
@@ -33,15 +32,6 @@ We also maintain a [gitter chat room][link_gitter] for more informal conversatio | |
There is significant cross-talk between these two spaces, and we look forward to hearing from you in either venue! | ||
As a reminder, we expect all contributions to `tedana` to adhere to our [code of conduct][link_coc]. | ||
|
||
### Monthly developer calls | ||
|
||
We run monthly developer calls via Zoom. | ||
You can see the schedule via the `tedana` [google calendar](https://calendar.google.com/calendar/embed?src=pl6vb4t9fck3k6mdo2mok53iss%40group.calendar.google.com). | ||
An agenda will be circulated in the gitter channel in advance of the meeting. | ||
|
||
Everyone is welcome. | ||
We look forward to meeting you there :hibiscus: | ||
|
||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
## Contributing small documentation changes | ||
If you are new to GitHub and just have a small documentation change | ||
recommendation, please submit it to [our e-mail address](mailto:[email protected]) | ||
|
@@ -92,11 +82,6 @@ towards ``tedana``'s shared vision. | |
We might have just missed it, or we might not (yet) see how it aligns with the overall project structure. | ||
These conversations are important to have, and we are excited to hear your perspective! | ||
|
||
* The **project board** is an automated [Kanban board][link_kanban] to keep track of what is currently underway | ||
(in progress), what has been completed (done), and what remains to be done for a specific release. | ||
The ``tedana`` maintainers use this board to keep an eye on how tasks are progressing week by week. | ||
|
||
|
||
### Issue labels | ||
|
||
The current list of labels are [here][link_labels] and include: | ||
|
@@ -151,30 +136,77 @@ Once you've run this, your repository should be set for most changes (i.e., you | |
|
||
### 4. Make the changes you've discussed | ||
|
||
Try to keep the changes focused to the issue. We've found that working on a [new branch][link_branches] for each issue makes it easier to keep your changes targeted. | ||
Try to keep the changes focused to the issue. | ||
We've found that working on a [new branch][link_branches] for each issue makes it easier to keep your changes targeted. | ||
Using a new branch allows you to follow the standard | ||
"fork/branch/commit/pull-request/merge" GitHub workflow when making changes. | ||
[This guide][link_gitworkflow] provides a useful overview for this workflow. | ||
Before making a new branch, make sure your master is up to date. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Using a new branch allows you to follow the standard "fork/branch/commit/pull-request/merge" GitHub workflow when making changes. [This guide][link_gitworkflow] provides a useful overview for this workflow. | ||
``` | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
git checkout master | ||
git fetch upstream master | ||
git merge upstream/master | ||
``` | ||
|
||
Then, make your new branch. | ||
|
||
``` | ||
git checkout -b MYBRANCH | ||
``` | ||
|
||
As you're making changes, | ||
make sure your branch is kept up to date with | ||
|
||
``` | ||
git fetch upstream master | ||
git merge upstream/master | ||
``` | ||
|
||
If you know what rebasing is, | ||
please only use it for changes that haven't been pushed. | ||
If you don't know what rebasing is, don't do it at all, | ||
as it is the easiest way to make your repository a disaster zone. | ||
Please make sure to review the `tedana` [style conventions](#style-guide) | ||
and test your changes. | ||
|
||
If you are new to ``git``, there are several GUI git clients that you may | ||
find helpful, such as | ||
- [GitKraken][link_git_kraken] | ||
- [GitHub Desktop][link_github_desktop] | ||
- [SourceTree][link_source_tree] | ||
|
||
Before creating your pull request, please make sure to review the `tedana` [style conventions](#style-guide). | ||
|
||
### 5. Test your changes | ||
emdupre marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#### Changes to code | ||
You can run style checks by running the following, | ||
``` | ||
flake8 $TEDANADIR/tedana | ||
``` | ||
|
||
For changes to the codebase, we suggest using our development Docker container which will run all the necessary checks and tests to ensure your code is ready to be merged into `tedana`! | ||
(This does require that you have a local install of [Docker](https://www.docker.com/products/docker-desktop).) | ||
You can run all the checks with: | ||
and unit/integration tests by running ``pytest``. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
If you know a file will test your change, | ||
you can run only that test. | ||
Alternatively, running all unit tests is relatively quick and should be | ||
fairly comprehensive. | ||
Running all ``pytest`` tests will be useful for pre-pushing checks. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
When you open a Pull Request, CircleCI will run all tests. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
``` | ||
docker run --tty --rm -v ${PWD}:/tedana tedana/tedana-dev:latest run_all_tests | ||
# All tests; final checks before pushing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer this section interwoven throughout rather than at the bottom. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I follow, sorry, could you elaborate? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think (in line with @emdupre's comment) it would be great to take each of the
and so on. |
||
pytest $TEDANADIR/tedana/tests | ||
# Unit tests and linting only | ||
pytest --skipintegration $TEDANADIR/tedana/tests | ||
# One test file in particular | ||
pytest $TEDANADIR/tedana/tests/test_file.py | ||
# Test one function in a file | ||
pytest -k my_function $TEDANADIR/tedana/tests/test_file.py | ||
``` | ||
|
||
from within your local `tedana` repository. | ||
(**N.B.** It is possible that, depending on your Docker setup, you may need to increase the amount of memory available to Docker in order to run the `tedana` test suite. | ||
You can either do this permanently by editing your Docker settings or temporarily by adding `--memory=4g` to the above `docker run` command.) | ||
|
||
This will print out a number of different status update messages as the tests run, but if you see `"FINISHED RUNNING ALL TESTS! GREAT SUCCESS"` then it means everything finished succesfully. | ||
If not, there should be some helpful outputs that specify which tests failed. | ||
The test run will indicate the number of passes and failures. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Most often, the failures are self-explanatory, | ||
but if not you can use the [pytest documentation][link_pytest] to use options to get more information. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#### Changes to documentation | ||
|
||
|
@@ -190,10 +222,18 @@ When opening the pull request, we ask that you follow some [specific conventions | |
|
||
After you have submitted the pull request, a member of the development team will review your changes to confirm that they can be merged into the main code base. | ||
|
||
After successful merging of the pull request, remember to [keep your fork up to date][link_updateupstreamwiki] with the master `tedana` repository and to delete the branch on your fork that was used for the merged pull request. | ||
|
||
### Pull Requests | ||
|
||
To push your changes to your remote, use | ||
|
||
``` | ||
git push -u origin MYBRANCH | ||
``` | ||
|
||
and GitHub will respond by giving you a link to open a pull request to | ||
ME-ICA/tedana. | ||
|
||
To improve understanding pull requests "at a glance", we encourage the use of several standardized tags. | ||
When opening a pull request, please use at least one of the following prefixes: | ||
|
||
|
@@ -202,20 +242,45 @@ When opening a pull request, please use at least one of the following prefixes: | |
* **[ENH]** for enhancements | ||
* **[FIX]** for bug fixes | ||
* **[REF]** for refactoring existing code | ||
* **[STY]** for stylistic changes | ||
* **[TST]** for new or updated tests, and | ||
* **[WIP]** for changes which are not yet ready to be merged | ||
* **[MAINT]** for maintenance of code | ||
|
||
You can also combine the tags above, for example if you are updating both a test and | ||
the documentation: **[TST, DOC]**. | ||
|
||
Pull requests should be submitted early and often! | ||
If your pull request is not yet ready to be merged, please also include the **[WIP]** prefix. | ||
If your pull request is not yet ready to be merged, please use [draft PRs][link_draftpr] | ||
This tells the development team that your pull request is a "work-in-progress", | ||
and that you plan to continue working on it. | ||
We request that you do not use the Draft PR feature at this time, | ||
as it interferes with our Continuous Integration tool, Travis. | ||
Note that if your pull request has no conversation or commits for 90 days, | ||
stale-bot will mark it stale. | ||
We may also ask that another development team member resume work on it, if you are unable to continue to do so. | ||
|
||
###Pull Request Checklist (Before Requesting Review): | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- [ ] Check that all tests are passing ("All tests passsed") | ||
- [ ] Make sure you have docstrings for any new functions | ||
- [ ] Make sure that docstrings are updated for edited functions | ||
- [ ] Make sure you note any issues that will be closed by your PR | ||
- [ ] Take a look at the automatically generated readthedocs for your PR (Show all checks -> continuous-documentation/readthedocs -> Details) | ||
|
||
### After Changes Are Merged | ||
After successful merging of the pull request, remember to [keep your fork up to date][link_updateupstreamwiki] with the master `tedana` repository and to delete the branch on your fork that was used for the merged pull request. | ||
You may also want to update your other branches to include these changes, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is redundant with the idea of merging in master, which I'd again encourage folks not to use unless there's a merge conflict. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Since we're suggesting as a first step in the contribution process that users fetch & merge upstream/master (before creating a new branch), I think we can safely remove this section! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I've just deleted those sections. |
||
which you can do via | ||
|
||
You can also combine the tags above, for example if you are updating both a test and | ||
the documentation: **[TST, DOC]**. | ||
If you're still working on the pull request that prefix would be **[WIP, TST, DOC]**. | ||
``` | ||
git checkout master | ||
git fetch upstream master | ||
git merge upstream/master | ||
git checkout OTHERBRANCH | ||
git merge master | ||
``` | ||
|
||
In the event of confusion, please ping in the Gitter for help. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's not recommend Gitter, here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤣 good point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternative of opening Draft PR and requesting help is proposed in 6aa2296 |
||
BE WARY OF WELL-INTENTIONED STACK OVERFLOW USERS TELLING YOU TO REBASE. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
### Advanced Development | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unclear what this means to a naive reader. Can you include examples (e.g., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that I have a great definition here; perhaps we could say something like, "Comprehensive Developer Guidelines" instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just renamed it "Comprehensive Developer Guidelines" in b7374a1 |
||
For core developers, please see our Developing Guidelines on [readthedocs][link_developing_rtd]. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## Style Guide | ||
|
||
|
@@ -289,6 +354,7 @@ You're awesome. :wave::smiley: | |
|
||
[link_kanban]: https://en.wikipedia.org/wiki/Kanban_board | ||
[link_pullrequest]: https://help.github.com/articles/creating-a-pull-request/ | ||
[link_draftpr]: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-requests#draft-pull-requests | ||
[link_fork]: https://help.github.com/articles/fork-a-repo/ | ||
[link_pushpullblog]: https://www.igvita.com/2011/12/19/dont-push-your-pull-requests/ | ||
[link_updateupstreamwiki]: https://help.github.com/articles/syncing-a-fork/ | ||
|
@@ -306,3 +372,9 @@ You're awesome. :wave::smiley: | |
[link_all-contributors-bot]: https://allcontributors.org/docs/en/bot/overview | ||
[link_all-contributors-bot-usage]: https://allcontributors.org/docs/en/bot/usage | ||
[link_stemmrolemodels]: https://github.com/KirstieJane/STEMMRoleModels | ||
[link_pytest]: https://docs.pytest.org/en/latest/usage.html | ||
[link_developing_rtd]: https://tedana.readthedocs.io/en/latest/developing.html | ||
|
||
[link_git_kraken]: https://www.gitkraken.com/ | ||
[link_github_desktop]: https://desktop.github.com/ | ||
[link_source_tree]: https://desktop.github.com/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,3 +8,4 @@ numpydoc | |
pytest | ||
pytest-cov | ||
requests | ||
sphinx |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
==================== | ||
Developer Guidelines | ||
==================== | ||
|
||
rmarkello marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Adding and Modifying Tests | ||
========================== | ||
Testing is an important component of development. | ||
For simplicity, we have migrated all tests to ``pytest``. | ||
There are two basic kinds of tests: | ||
unit and integration tests. | ||
Unit tests focus on testing individual functions, | ||
whereas integration tests focus on making sure that the whole workflow | ||
runs correctly. | ||
|
||
For unit tests, | ||
we try to keep tests on the same module grouped into one file. | ||
Make sure the function you're testing is imported, | ||
then write your test. | ||
Good tests will make sure that edge cases are accounted for as well as | ||
common cases. | ||
You may also use ``pytest.raises`` to ensure that errors are thrown for | ||
invalid inputs to a function. | ||
|
||
For integration tests, | ||
make a ``tar.gz`` file which will unzip to be a single directory | ||
containing all of the files you'd like to run a workflow on. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want the
the appropriate tar command would be: tar czf my_data.tar.gz my_data/*.nii.gz Thus, when untarred (
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant to type "archive," whoops. Addressed in a63ccfe |
||
Run the workflow with a known-working version, and put the outputs into a | ||
text file inside ``$TEDANADIR/tedana/tests/data/``, | ||
with ``TEDANADIR`` the local ``tedana repository``. | ||
You can follow the model our `five echo set`_, | ||
which has the following steps: | ||
|
||
1. Check if a pytest user is skipping integration, skip if so | ||
#. Use ``download_test_data`` to retrieve the test data from OSF | ||
#. Run a workflow | ||
#. Use ``resources_filename`` and ``check_integration_outputs`` to compare your expected output to actual | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
If you need to upload new data, you will need to contact the maintainers | ||
and ask them to either add it or give you permission to add it. | ||
Once you've tested your integration test locally and it is working, | ||
you will need to add it to the CircleCI config and the ``Makefile``. | ||
Following the model of the three-echo and five-echo sets, | ||
define a name for your integration test and on an indented line below put | ||
|
||
.. code-block:: none | ||
|
||
@py.test --cov-append --cov-report term-missing --cov=tedana -k TEST | ||
with ``TEST`` your test function's name. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
This call basically adds code coverage reports to account for the new test, | ||
and runs the actual test in addition. | ||
Using the five-echo set as a template, | ||
you should then edit ``.circlec/config.yml`` to add your test, | ||
calling the same name you define in the ``Makefile``. | ||
|
||
If you need to take a look at a failed test on CircleCI rather than | ||
locally, you can use the following block to retrieve artifacts | ||
(see CircleCI documentation here_) | ||
|
||
.. code-block:: none | ||
export CIRCLE_TOKEN=':your_token' | ||
|
||
curl https://circleci.com/api/v1.1/project/:vcs-type/:username/:project/$build_number/artifacts?circle-token=$CIRCLE_TOKEN \ | ||
| grep -o 'https://[^"]*' \ | ||
| sed -e "s/$/?circle-token=$CIRCLE_TOKEN/" \ | ||
| wget -v -i - | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, I think people (who, as you note, are part of the ME-ICA org) can just log on to the CircleCI UI and navigate to the artifacts tab if they're rather only download one or two files rather than all of them. Perhaps it would be worth noting this! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in d722317 |
||
|
||
To get a CircleCI token, follow the instructions for `getting one`_. | ||
You cannot do this unless you are part of the ME-ICA/tedana organization. | ||
|
||
Worked Example | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
============== | ||
Suppose that a | ||
|
||
.. _git: https://git-scm.com/ | ||
.. _`git pro`: https://git-scm.com/book/en/v2 | ||
.. _repository: https://github.com/ME-ICA/tedana | ||
.. _Fork: https://help.github.com/en/github/getting-started-with-github/fork-a-repo | ||
.. _`pull request`: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request | ||
.. _GitKraken: https://www.gitkraken.com/ | ||
.. _`GitHub Desktop`: https://desktop.github.com/ | ||
.. _SourceTree: https://www.sourcetreeapp.com/ | ||
.. _`GitHub UI`: https://help.github.com/en/github/managing-files-in-a-repository/editing-files-in-your-repository | ||
.. _this: https://github.com/ME-ICA/tedana/tree/master/docs | ||
.. _ReStructuredText: http://docutils.sourceforge.net/rst.html#user-documentation | ||
.. _`five echo set`: https://github.com/ME-ICA/tedana/blob/37368f802f77b4327fc8d3f788296ca0f01074fd/tedana/tests/test_integration.py#L71-L95 | ||
.. _here: https://circleci.com/docs/2.0/artifacts/#downloading-all-artifacts-for-a-build-on-circleci | ||
.. _`getting one`: https://circleci.com/docs/2.0/managing-api-tokens/?gclid=CjwKCAiAqqTuBRBAEiwA7B66heDkdw6l68GAYAHtR2xS1xvDNNUzy7l1fmtwQWvVN0OIa97QL8yfhhoCejoQAvD_BwE#creating-a-personal-api-token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very useful section, I think there are a couple of other points in the document readers might want to quickly navigate to, so I would add:
I would also link to the developer docs here with a brief description of the contents, say:
See also our Developing Guidelines on [readthedocs][link_developing_rtd] for additional information, particularly on our monthly developer calls and writing tests for tedana.