-
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 9 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: | ||
|
@@ -159,22 +144,30 @@ Before creating your pull request, please make sure to review the `tedana` [styl | |
|
||
### 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 | ||
|
||
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: | ||
``` | ||
cd TEDANADIR | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
flake8 tedana | ||
``` | ||
|
||
and unit/integration tests by running the following: | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
docker run --tty --rm -v ${PWD}:/tedana tedana/tedana-dev:latest run_all_tests | ||
cd TEDANADIR/tedana/tests | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# All tests | ||
pytest . | ||
# Unit tests and linting only (saves time) | ||
pytest --skipintegration . | ||
# One test file in particular | ||
pytest test_file.py | ||
# Test one function in a file | ||
pytest -k my_function 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 | ||
|
||
|
@@ -202,20 +195,28 @@ 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 and we may ask your permission to take over it. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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]**. | ||
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) | ||
|
||
### 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 +290,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 +308,5 @@ 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 |
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,155 @@ | ||
Developer Guidelines | ||
==================== | ||
|
||
rmarkello marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Git Workflow | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
------------ | ||
.. _`git workflow`: | ||
When making changes to the code or large documentation changes, you will want to use the following workflow: | ||
|
||
Make sure you're up to date: | ||
|
||
.. code-block:: none | ||
|
||
git checkout master | ||
git fetch upstream/master | ||
git merge upstream/master | ||
|
||
If you are asked to make a merge commit here, | ||
something strange has happened. | ||
Ping people in Gitter for help. | ||
Most likely, you have rebased your master branch and you will need to reset it. | ||
When your local master is up to date, you'll want to make a new branch: | ||
|
||
.. code-block:: none | ||
|
||
git checkout -b MYBRANCH | ||
|
||
Make your changes as desired, | ||
periodically merging in new changes in master via: | ||
|
||
.. code-block:: none | ||
|
||
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. | ||
To push your changes: | ||
|
||
.. code-block:: none | ||
|
||
git push -u origin MYBRANCH | ||
GitHub very helpfully responds to pushes by giving you a link to open a | ||
Pull Request to ME-ICA/tedana, if you look in the lines starting with | ||
``remote:``, so you may want to go ahead and use the link. | ||
Once your change is merged, you'll want to do the following, | ||
which will update your local master and delete your branch: | ||
|
||
.. code-block:: none | ||
|
||
git checkout master | ||
git fetch upstream/master | ||
git merge upstream/master | ||
git branch -d MYBRANCH | ||
You may need to update other branches after this, which you can do via: | ||
|
||
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 would add a link to your contributing guide somewhere up here |
||
.. code-block:: none | ||
|
||
git checkout OTHERBRANCH | ||
git merge upstream/master | ||
In the event of confusion, please ping in the Gitter for help. | ||
BE WARY OF WELL-INTENTIONED STACK OVERFLOW USERS TELLING YOU TO REBASE. | ||
|
||
NOTE: if you don't understand the above steps, consider using a git tutorial and a practice repository until you get the hang of it. | ||
Feel free also to ask questions and use a ``git`` client. | ||
Several popular ones include | ||
|
||
- GitKraken_ | ||
- `GitHub Desktop`_ | ||
- SourceTree_ | ||
|
||
|
||
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/``. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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.