From 111cd6b4bc9a0bcbd1ed287dcb0c4a9d7a0ccf80 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Sat, 9 Nov 2019 01:19:47 -0500 Subject: [PATCH 01/59] Adds developer guideline start --- docs/developing.rst | 87 +++++++++++++++++++++++++++++++++++++++++++++ docs/index.rst | 1 + 2 files changed, 88 insertions(+) create mode 100644 docs/developing.rst diff --git a/docs/developing.rst b/docs/developing.rst new file mode 100644 index 000000000..0e17f3026 --- /dev/null +++ b/docs/developing.rst @@ -0,0 +1,87 @@ +Developer Guidelines +==================== + +Basics +------ +.. _basics: +``tedana`` uses git_ version control on the popular open source platform GitHub. +This allows the developers to easily manage changes even if they're very complex. +The most comprehensive tutorial on using git_ is `git pro`_. +For `small documentation patches`_ read the below section; otherwise, you will need to have a basic understanding of git. +When making code changes, you will use the `git workflow`_ described. +Once you're happy with your progress and open a pull request, a team member will review your changes via GitHub. +They may ask you to make changes or make suggestions. +Additionally, your changes will trigger automatic testing. +If your tests fail, you can inspect their results to see the cause of failure. +Common failures include: + +- Linting (code style; we adhere to PEP8) +- Unit Test failure (a module does not work as expected) +- Integration Test failure (a change causes the program as a whole to fail) + +If all tests are successful, you can squash and merge your changes with an approving review. +Thanks for your contribution! + + +Git Workflow +------------ +.. _`git workflow`: +When making changes to the code or large documentation changes, you will want to use the following workflow: + +1. Fork_ the repository_ on GitHub. +#. Create a new branch locally. +#. Frequently fetch and merge the ``tedana`` master branch into your work (this avoids merge conflicts). +#. Open a `pull request`_ to the ``tedana`` project. + +There are a variety of ways to do this on a GUI'd ``git`` client. +If you're using the command line and ssh keys, the following will approximately follow the above: + +.. code-block:: none + + # clone your fork + git clone git@github.com:YOUR_PROFILE/tedana.git + # enter your fork + cd tedana + # add the repository as an upstream + git remote add upstream git@github.com:ME-ICA/tedana.git + # create a new feature branch + git checkout -b feature/my_awesome_contribution + # work, work, work, ... + # stay up to date + git fetch upstream + git merge upstream/master + # push to your remote + git push -u origin feature/my_awesome_contribution + # the command line makes a link you can use to open a pull request + # maybe you'll continue working + git push + # your pull request is now up-to-date + +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_ + +Small Documentation Patches +--------------------------- +.. _`small documentation patches`: +You can use the `GitHub UI`_ to make small documentation patches directly on the repository. +Please create a new branch as part of step 7 in the linked article. +Note that the documentation is mostly hosted inside of this_ folder using ReStructuredText_. + + + +.. _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 diff --git a/docs/index.rst b/docs/index.rst index 7f02eb45e..2cf2bc8cd 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -152,6 +152,7 @@ tedana is licensed under GNU Lesser General Public License version 2.1. faq support contributing + developing roadmap api From 78914071e68d729f1242a147d39567b632217842 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 12:05:57 -0500 Subject: [PATCH 02/59] Modifies CONTRIBUTING --- CONTRIBUTING.md | 54 +++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 314bd9b46..059253860 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -33,15 +33,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: - ## 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:tedana.devs@gmail.com) @@ -161,20 +152,24 @@ Before creating your pull request, please make sure to review the `tedana` [styl #### Changes to code -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: +You can run all the checks from running the following ``` -docker run --tty --rm -v ${PWD}:/tedana tedana/tedana-dev:latest run_all_tests +cd TEDANADIR/tedana/tests +# 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. +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. #### Changes to documentation @@ -202,20 +197,25 @@ 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. -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): +- [ ] 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) ## Style Guide @@ -289,6 +289,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 +307,4 @@ 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 From 36cf608d89288c4f62c291d343b36ec7eb4fcfb7 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 12:10:00 -0500 Subject: [PATCH 03/59] Fixes dead link, removes project board --- CONTRIBUTING.md | 8 -------- 1 file changed, 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 059253860..1e72a47c6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -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) @@ -83,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: @@ -150,8 +144,6 @@ Before creating your pull request, please make sure to review the `tedana` [styl ### 5. Test your changes -#### Changes to code - You can run all the checks from running the following ``` From d1d945cc5b44f7484c411def83eef9685b97413d Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 13:16:53 -0500 Subject: [PATCH 04/59] Adds more thorough developing guidelines --- CONTRIBUTING.md | 4 ++ docs/developing.rst | 149 +++++++++++++++++++++++++++++--------------- 2 files changed, 102 insertions(+), 51 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1e72a47c6..05f2b69db 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -209,6 +209,9 @@ Pull Request Checklist (Before Requesting Review): - [ ] 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 +For core developers, please see our Developing Guidelines on [readthedocs][link_developing_rtd]. + ## Style Guide Docstrings should follow [numpydoc][link_numpydoc] convention. @@ -300,3 +303,4 @@ You're awesome. :wave::smiley: [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 diff --git a/docs/developing.rst b/docs/developing.rst index 0e17f3026..573e00941 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -1,61 +1,66 @@ Developer Guidelines ==================== -Basics ------- -.. _basics: -``tedana`` uses git_ version control on the popular open source platform GitHub. -This allows the developers to easily manage changes even if they're very complex. -The most comprehensive tutorial on using git_ is `git pro`_. -For `small documentation patches`_ read the below section; otherwise, you will need to have a basic understanding of git. -When making code changes, you will use the `git workflow`_ described. -Once you're happy with your progress and open a pull request, a team member will review your changes via GitHub. -They may ask you to make changes or make suggestions. -Additionally, your changes will trigger automatic testing. -If your tests fail, you can inspect their results to see the cause of failure. -Common failures include: - -- Linting (code style; we adhere to PEP8) -- Unit Test failure (a module does not work as expected) -- Integration Test failure (a change causes the program as a whole to fail) - -If all tests are successful, you can squash and merge your changes with an approving review. -Thanks for your contribution! - - Git Workflow ------------ .. _`git workflow`: When making changes to the code or large documentation changes, you will want to use the following workflow: -1. Fork_ the repository_ on GitHub. -#. Create a new branch locally. -#. Frequently fetch and merge the ``tedana`` master branch into your work (this avoids merge conflicts). -#. Open a `pull request`_ to the ``tedana`` project. +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 -There are a variety of ways to do this on a GUI'd ``git`` client. -If you're using the command line and ssh keys, the following will approximately follow the above: +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 - # clone your fork - git clone git@github.com:YOUR_PROFILE/tedana.git - # enter your fork - cd tedana - # add the repository as an upstream - git remote add upstream git@github.com:ME-ICA/tedana.git - # create a new feature branch - git checkout -b feature/my_awesome_contribution - # work, work, work, ... - # stay up to date - git fetch upstream + 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: + +.. code-block:: none + + git checkout OTHERBRANCH git merge upstream/master - # push to your remote - git push -u origin feature/my_awesome_contribution - # the command line makes a link you can use to open a pull request - # maybe you'll continue working - git push - # your pull request is now up-to-date +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. @@ -65,14 +70,55 @@ Several popular ones include - `GitHub Desktop`_ - SourceTree_ -Small Documentation Patches ---------------------------- -.. _`small documentation patches`: -You can use the `GitHub UI`_ to make small documentation patches directly on the repository. -Please create a new branch as part of step 7 in the linked article. -Note that the documentation is mostly hosted inside of this_ folder using ReStructuredText_. +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. +Run the workflow with a known-working version, and put the outputs into a +text file inside ``TEDANADIR/tedana/tests/data/``. +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 + +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. +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``. .. _git: https://git-scm.com/ .. _`git pro`: https://git-scm.com/book/en/v2 @@ -85,3 +131,4 @@ Note that the documentation is mostly hosted inside of this_ folder using ReStru .. _`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 From 18a2e6b2c1ad21c51a8d7efd379b532300419547 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 13:32:16 -0500 Subject: [PATCH 05/59] Fix lack of linting --- CONTRIBUTING.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 05f2b69db..e4a3f877f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -149,7 +149,8 @@ You can run all the checks from running the following ``` cd TEDANADIR/tedana/tests # All tests -pytest . +# --flake8 runs linting +pytest --flake8 . # Unit tests and linting only (saves time) pytest --skipintegration . # One test file in particular From ff70cacc114a5af0b39f5865b78293134b58aaa1 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 15:20:02 -0500 Subject: [PATCH 06/59] Updates dev requirements --- dev_requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/dev_requirements.txt b/dev_requirements.txt index 608d2780b..4e0e0881c 100644 --- a/dev_requirements.txt +++ b/dev_requirements.txt @@ -8,3 +8,4 @@ numpydoc pytest pytest-cov requests +sphinx From e7e542ebeab19cc3063be5d865d664dc39697e2a Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 15:25:49 -0500 Subject: [PATCH 07/59] Adds flake8 instructions --- CONTRIBUTING.md | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e4a3f877f..f261523a5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -144,13 +144,18 @@ Before creating your pull request, please make sure to review the `tedana` [styl ### 5. Test your changes -You can run all the checks from running the following +You can run style checks by running the following +``` +cd TEDANADIR +flake8 tedana +``` + +and unit/integration tests by running the following: ``` cd TEDANADIR/tedana/tests # All tests -# --flake8 runs linting -pytest --flake8 . +pytest . # Unit tests and linting only (saves time) pytest --skipintegration . # One test file in particular From ae88853d7c124f253f5dd071b24d17166068940c Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 15:36:32 -0500 Subject: [PATCH 08/59] Adds instructions for retrieving CircleCI artifacts --- docs/developing.rst | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/developing.rst b/docs/developing.rst index 573e00941..680665cdb 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -120,6 +120,25 @@ 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 - + +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 +-------------- +Suppose that a + .. _git: https://git-scm.com/ .. _`git pro`: https://git-scm.com/book/en/v2 .. _repository: https://github.com/ME-ICA/tedana @@ -132,3 +151,5 @@ calling the same name you define in the ``Makefile``. .. _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 From 090f786804a5be568ee90d0ea102793b70dd7873 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 15:37:00 -0500 Subject: [PATCH 09/59] Updates config with correct CI version --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 6c834a581..5ae4aa2cf 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,4 +1,4 @@ -# Python CircleCI 2.0 configuration file +# Python CircleCI 2.1 configuration file # # Check https://circleci.com/docs/2.0/language-python/ for more details # From f5cc4d76165c8f389faee7273b651e4cb1a8cf80 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 17:33:11 -0500 Subject: [PATCH 10/59] Address @emdupre review --- CONTRIBUTING.md | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f261523a5..7f192d649 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -144,19 +144,25 @@ Before creating your pull request, please make sure to review the `tedana` [styl ### 5. Test your changes -You can run style checks by running the following - +You can run style checks by running the following, ``` -cd TEDANADIR +cd $TEDANADIR flake8 tedana ``` -and unit/integration tests by running the following: +and unit/integration tests by running ``pytest``. +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. +When you open a Pull Request, CircleCI will run all tests. + ``` cd TEDANADIR/tedana/tests -# All tests +# All tests; final checks before pushing pytest . -# Unit tests and linting only (saves time) +# Unit tests and linting only pytest --skipintegration . # One test file in particular pytest test_file.py From 74b7462458d4402469ce6cf5a56dc8df91088609 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 17:39:09 -0500 Subject: [PATCH 11/59] Addresses a different @emdupre review --- CONTRIBUTING.md | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7f192d649..e11453bfc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -146,8 +146,7 @@ Before creating your pull request, please make sure to review the `tedana` [styl You can run style checks by running the following, ``` -cd $TEDANADIR -flake8 tedana +flake8 $TEDANADIR/tedana ``` and unit/integration tests by running ``pytest``. @@ -159,15 +158,14 @@ Running all ``pytest`` tests will be useful for pre-pushing checks. When you open a Pull Request, CircleCI will run all tests. ``` -cd TEDANADIR/tedana/tests # All tests; final checks before pushing -pytest . +pytest $TEDANADIR/tedana/tests # Unit tests and linting only -pytest --skipintegration . +pytest --skipintegration $TEDANADIR/tedana/tests # One test file in particular -pytest test_file.py +pytest $TEDANADIR/tedana/tests/test_file.py # Test one function in a file -pytest -k my_function test_file.py +pytest -k my_function $TEDANADIR/tedana/tests/test_file.py ``` from within your local `tedana` repository. From 079a41a3710e329ef8df058688e87c340788e419 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 17:39:27 -0500 Subject: [PATCH 12/59] Update CONTRIBUTING.md Co-Authored-By: Elizabeth DuPre --- CONTRIBUTING.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f261523a5..6d25b1457 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -206,7 +206,8 @@ If your pull request is not yet ready to be merged, please use [draft PRs][link_ This tells the development team that your pull request is a "work-in-progress", and that you plan to continue working on it. 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. +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): - [ ] Check that all tests are passing ("All tests passsed") From 79e7b527722c22f412b86bee7f1c4b6580599605 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 17:46:23 -0500 Subject: [PATCH 13/59] More addressing --- CONTRIBUTING.md | 2 +- docs/developing.rst | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e11453bfc..0de7431eb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -212,7 +212,7 @@ and that you plan to continue working on it. 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. -Pull Request Checklist (Before Requesting Review): +###Pull Request Checklist (Before Requesting Review): - [ ] 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 diff --git a/docs/developing.rst b/docs/developing.rst index 680665cdb..7f6a0e91f 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -1,8 +1,9 @@ +==================== Developer Guidelines ==================== Git Workflow ------------- +============ .. _`git workflow`: When making changes to the code or large documentation changes, you will want to use the following workflow: @@ -72,7 +73,7 @@ Several popular ones include 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: @@ -94,7 +95,8 @@ 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. Run the workflow with a known-working version, and put the outputs into a -text file inside ``TEDANADIR/tedana/tests/data/``. +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: @@ -136,7 +138,7 @@ 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 --------------- +============== Suppose that a .. _git: https://git-scm.com/ From 94799a7b190fcab17d0c9ee4cf3db46dadec7e59 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 11 Nov 2019 18:02:18 -0500 Subject: [PATCH 14/59] More review addressing --- CONTRIBUTING.md | 71 ++++++++++++++++++++++++++++++++++++++++++--- docs/developing.rst | 70 -------------------------------------------- 2 files changed, 67 insertions(+), 74 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 78caaeea3..8f6b0edf2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -136,11 +136,46 @@ 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. -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. +``` +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 @@ -187,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: @@ -220,6 +263,22 @@ We may also ask that another development team member resume work on it, if you a - [ ] 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, +which you can do via + +``` +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. +BE WARY OF WELL-INTENTIONED STACK OVERFLOW USERS TELLING YOU TO REBASE. + ### Advanced Development For core developers, please see our Developing Guidelines on [readthedocs][link_developing_rtd]. @@ -315,3 +374,7 @@ You're awesome. :wave::smiley: [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/ diff --git a/docs/developing.rst b/docs/developing.rst index 7f6a0e91f..2123d8982 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -2,76 +2,6 @@ Developer Guidelines ==================== -Git Workflow -============ -.. _`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: - -.. 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. From 6a3043fda4319ebc8d90db46a38f99c5cf4f6eda Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Tue, 12 Nov 2019 23:03:28 -0500 Subject: [PATCH 15/59] Update CONTRIBUTING.md Co-Authored-By: Ross Markello --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8f6b0edf2..43ea4aa85 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -277,7 +277,7 @@ git merge 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. +We strongly recommend you *not* use the `git rebase` command when working with and contributing to the `tedana` repository. ### Advanced Development For core developers, please see our Developing Guidelines on [readthedocs][link_developing_rtd]. From db327ff38c2c7082fa4b0561f3162b58930aaf80 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Tue, 12 Nov 2019 23:03:50 -0500 Subject: [PATCH 16/59] Update CONTRIBUTING.md Co-Authored-By: Ross Markello --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 43ea4aa85..e165327d6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -184,7 +184,7 @@ You can run style checks by running the following, flake8 $TEDANADIR/tedana ``` -and unit/integration tests by running ``pytest``. +and unit/integration tests by running `pytest`. 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 From 558b74d92770805f04f6245f6d538af4d43f1f0d Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Tue, 12 Nov 2019 23:04:01 -0500 Subject: [PATCH 17/59] Update CONTRIBUTING.md Co-Authored-By: Ross Markello --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e165327d6..cc748cfe1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -189,7 +189,7 @@ 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. +Running all `pytest` tests will be useful for pre-pushing checks. When you open a Pull Request, CircleCI will run all tests. ``` From 30d125a3effd677845ad5634370e33c32b40c52b Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Tue, 12 Nov 2019 23:04:10 -0500 Subject: [PATCH 18/59] Update docs/developing.rst Co-Authored-By: Ross Markello --- docs/developing.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/developing.rst b/docs/developing.rst index 2123d8982..1212f02e0 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -45,6 +45,7 @@ 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. This call basically adds code coverage reports to account for the new test, and runs the actual test in addition. From 95921d4cdd9895d6d9ff82de950615e77ed76783 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Tue, 12 Nov 2019 23:04:23 -0500 Subject: [PATCH 19/59] Update CONTRIBUTING.md Co-Authored-By: Ross Markello --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cc748cfe1..5d9cf2c4b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -280,7 +280,7 @@ In the event of confusion, please ping in the Gitter for help. We strongly recommend you *not* use the `git rebase` command when working with and contributing to the `tedana` repository. ### Advanced Development -For core developers, please see our Developing Guidelines on [readthedocs][link_developing_rtd]. +For additional, in-depth information on contributing to `tedana`, please see our Developing Guidelines on [readthedocs][link_developing_rtd]. ## Style Guide From a535ee9db33eef764d42e8adc42daffc4cdd3fb4 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Tue, 12 Nov 2019 23:08:12 -0500 Subject: [PATCH 20/59] Adds manual selection of artifiacts through CircleCI UI --- docs/developing.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/developing.rst b/docs/developing.rst index 2123d8982..e42f7e52c 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -66,6 +66,8 @@ locally, you can use the following block to retrieve artifacts 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. +If you don't want all of the artifacts, you can go to the test details and use the browser to +manually select the files you would like. Worked Example ============== From a63ccfed5ffb98b88cc6a1f62b4efad9a55bc6b9 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Tue, 12 Nov 2019 23:12:51 -0500 Subject: [PATCH 21/59] Adds tar command --- docs/developing.rst | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/developing.rst b/docs/developing.rst index 328aa26bd..ac50259e3 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -22,8 +22,12 @@ 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. +make a ``tar.gz`` file which will unzip to be only the files you'd like to run a workflow on. +You can do this with the following, which would make an archive ``my_data.tar.gz``: + +.. code-block:: none + tar czf my_data.tar.gz my_data/*.nii.gz + 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``. From aa41ffe1275234a47265a482eb8bbec50fd8dc1c Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Tue, 12 Nov 2019 23:16:00 -0500 Subject: [PATCH 22/59] Adds tar command --- docs/developing.rst | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/developing.rst b/docs/developing.rst index ac50259e3..454ceff28 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -78,6 +78,14 @@ Worked Example ============== Suppose that a +Monthly Developer Calls +======================= +We run monthly developer calls via Zoom. +You can see the schedule via the tedana `google calendar`_. + +Everyone is welcome. +We look forward to meeting you there :hibiscus: + .. _git: https://git-scm.com/ .. _`git pro`: https://git-scm.com/book/en/v2 .. _repository: https://github.com/ME-ICA/tedana @@ -92,3 +100,4 @@ Suppose that a .. _`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 +.. _`google calendar`: https://calendar.google.com/calendar/embed?src=pl6vb4t9fck3k6mdo2mok53iss%40group.calendar.google.com From 8beeecdbb0cbd6b37565c04cd170bbbf80aa2930 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Wed, 13 Nov 2019 15:24:18 -0500 Subject: [PATCH 23/59] Fix formatting --- docs/developing.rst | 78 +++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 42 deletions(-) diff --git a/docs/developing.rst b/docs/developing.rst index 454ceff28..3f4e98a69 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -6,62 +6,56 @@ 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 only the files you'd like to run a workflow on. +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 only the files you'd like to +run a workflow on. You can do this with the following, which would make an archive ``my_data.tar.gz``: -.. code-block:: none +.. code-block:: bash + tar czf my_data.tar.gz my_data/*.nii.gz -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: +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 +#. Use ``resources_filename`` and ``check_integration_outputs`` to compare your expected output to + actual output. -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 +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 +.. code-block:: bash @py.test --cov-append --cov-report term-missing --cov=tedana -k TEST -with ``TEST`` your test function's name. -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``. +with ``TEST`` your test function's name. +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_) -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:: bash -.. 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 \ @@ -76,7 +70,7 @@ manually select the files you would like. Worked Example ============== -Suppose that a +Suppose that a Monthly Developer Calls ======================= From aa621c96dfe0f0d1beafd3772f719c2451d181b4 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Wed, 13 Nov 2019 16:01:41 -0500 Subject: [PATCH 24/59] Adds example --- docs/developing.rst | 86 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 1 deletion(-) diff --git a/docs/developing.rst b/docs/developing.rst index 3f4e98a69..cb8f2673b 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -70,7 +70,91 @@ manually select the files you would like. Worked Example ============== -Suppose that a +Suppose we want to add a function in ``tedana`` that creates a file called ```hello_world.txt`` to +be stored along the outputs of the ``tedana`` workflow. + +First, we merge the repository's ``master`` branch into our own to make sure we're up to date, and +then we make a new branch called something like ``feature/say_hello``. +Any changes we make will stay on this branch. +We make the new function and call it ``say_hello`` and locate this function inside of ``io.py``. +We'll also need to make a unit test. +(Some developers actually make the unit test before the new function; this is a great way to make +sure you don't forget to create it!) +Since the function lives in ``io.py``, its unit test should go into ``test_io.py``. +The job of this test is exclusively to tell if the function we wrote does what it claims to do +without errors. +So, we define a new function in ``test_io.py`` that looks something like this: + +.. code-block:: python + + def test_say_hello(): + # run the function + say_hello() + # test the function + assert op.exists('hello_world.txt') + # clean up + os.remove('hello_world.txt') + +We should see that ``pytest test_io.py`` is successful. +If not, we should continue editing the function until it passes our test. +Let's suppose that suddenly, you realize that what would be even more useful is a function that +takes an argument, ``place``, so that the output filename is actually ``hello_PLACE``, with +``PLACE`` the value passed and ``'world'`` as the default value. +We merge any changes from the upstream master branch into our branch via + +.. code-block:: bash + git checkout feature/say_hello # unless you're already there + git fetch upstream master + git merge upstream/master + +and then begin work on our test. +We need to our unit test to be more complete, so we update it to look more like the following, +adding several cases to make sure our function is robust to the name supplied: + +.. code-block:: python + + def test_say_hello(): + # prefix of all files to be checked + prefix = 'hello_' + # suffix of all files to be checked + suffix = '.txt' + # run the function with several cases + for x in ['world', 'solar system', 'galaxy', 'universe']: + # current test name + outname = prefix + x + suffix + # call the function + say_hello(x) + # test the function + assert op.exists(outname) + # clean up from this call + os.remove(outname) + +Once that test is passing, we may need to adjust the integration test. +Our program creates a file, ``hello_world.txt``, which the older version would not have produced. +Therefore, we need to add the file to ``$TEDANADIR/tedana/tests/data/tedana_outputs.txt`` and its +counterpart, R2-D2-- uh, we mean, ``tedana_outputs_verbose.txt``. +Once that filename is added, all of the tests should be passing and we should open a PR to have our +change reviewed. + +From here, others working on the project may request changes and we'll have to make sure that our +tests are kept up to date with any changes made as we did before updating the unit test. +For example, if a new parameter is added, ``greeting``, with a default of ``hello``, we'll need to +adjust the unit test. +However, since this doesn't change the typical workflow of ``tedana``, there's no need to change +the integration test; we're still matching the original filename. +Once we are happy with the changes and some members of ``tedana`` have approved the changes, our +changes will be merged! + +We should then do the following cleanup with our git repository: + +.. clode-blocks:: bash + git checkout master + git fetch upstream master + git merge upstream/master + git branch -d feature/say_hello + git push --delete origin feature/say_hello + +and we're good to go! Monthly Developer Calls ======================= From a03f095279c2e57d6dc86acac14503cfa0c0ac8a Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Wed, 13 Nov 2019 16:05:06 -0500 Subject: [PATCH 25/59] Stupid line break typo --- docs/developing.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/developing.rst b/docs/developing.rst index cb8f2673b..07e4bfa17 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -103,6 +103,7 @@ takes an argument, ``place``, so that the output filename is actually ``hello_PL We merge any changes from the upstream master branch into our branch via .. code-block:: bash + git checkout feature/say_hello # unless you're already there git fetch upstream master git merge upstream/master @@ -147,7 +148,8 @@ changes will be merged! We should then do the following cleanup with our git repository: -.. clode-blocks:: bash +.. code-block:: bash + git checkout master git fetch upstream master git merge upstream/master From 93c465b0ef43715b25a36a4b60e75ac95ac9a916 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Wed, 13 Nov 2019 16:10:39 -0500 Subject: [PATCH 26/59] Clarify some pytest stuff --- docs/developing.rst | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/docs/developing.rst b/docs/developing.rst index 07e4bfa17..46c8463c6 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -95,7 +95,12 @@ So, we define a new function in ``test_io.py`` that looks something like this: # clean up os.remove('hello_world.txt') -We should see that ``pytest test_io.py`` is successful. +We should see that our unit test is successful via + +.. code-block:: bash + + pytest $TEDANADIR/tedana/tests/test_io.py -k test_say_hello + If not, we should continue editing the function until it passes our test. Let's suppose that suddenly, you realize that what would be even more useful is a function that takes an argument, ``place``, so that the output filename is actually ``hello_PLACE``, with @@ -134,6 +139,12 @@ Once that test is passing, we may need to adjust the integration test. Our program creates a file, ``hello_world.txt``, which the older version would not have produced. Therefore, we need to add the file to ``$TEDANADIR/tedana/tests/data/tedana_outputs.txt`` and its counterpart, R2-D2-- uh, we mean, ``tedana_outputs_verbose.txt``. +With that edit complete, we can run the full ``pytest`` suite via + +.. code-block:: bash + + pytest $TEDANADIR/tedana/tests + Once that filename is added, all of the tests should be passing and we should open a PR to have our change reviewed. From 9ad1eca95a215eea71642a21657cfbb14272c53e Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Wed, 13 Nov 2019 16:20:51 -0500 Subject: [PATCH 27/59] Address @rmarkello review --- docs/developing.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/developing.rst b/docs/developing.rst index 46c8463c6..e26a9a8c1 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -2,6 +2,11 @@ Developer Guidelines ==================== +This webpage is intended to guide users through making making changes to +``tedana``'s codebase, in particular working with tests. +The worked example also offers some guidelines on approaching testing when +adding new functions. + Adding and Modifying Tests ========================== Testing is an important component of development. From 1c152640e980b9540b680f6317b4d07688bcc3fe Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Wed, 13 Nov 2019 16:26:35 -0500 Subject: [PATCH 28/59] Addresss @rmarkello review --- CONTRIBUTING.md | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5d9cf2c4b..92356c7fe 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -136,10 +136,10 @@ 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. -Using a new branch allows you to follow the standard -"fork/branch/commit/pull-request/merge" GitHub workflow when making changes. +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. @@ -155,23 +155,19 @@ Then, make your new branch. git checkout -b MYBRANCH ``` -As you're making changes, -make sure your branch is kept up to date with +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 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 +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] @@ -179,14 +175,13 @@ find helpful, such as ### 5. Test your changes -You can run style checks by running the following, +You can run style checks by running the following: ``` flake8 $TEDANADIR/tedana ``` -and unit/integration tests by running `pytest`. -If you know a file will test your change, -you can run only that test. +and unit/integration tests by running `pytest` (more details below). +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. @@ -205,8 +200,8 @@ pytest -k my_function $TEDANADIR/tedana/tests/test_file.py from within your local `tedana` repository. The test run will indicate the number of passes and failures. -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. +Most often, the failures give enough information to determine the cause; if not, if not you can +refer to the [pytest documentation][link_pytest] for more details on the failure. #### Changes to documentation From d1af26b310b2c4b26448dc6d3e24df30248317a3 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Thu, 14 Nov 2019 17:17:31 -0500 Subject: [PATCH 29/59] Update CONTRIBUTING.md Co-Authored-By: Ross Markello --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 92356c7fe..e48b9c725 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -200,7 +200,7 @@ pytest -k my_function $TEDANADIR/tedana/tests/test_file.py from within your local `tedana` repository. The test run will indicate the number of passes and failures. -Most often, the failures give enough information to determine the cause; if not, if not you can +Most often, the failures give enough information to determine the cause; if not, you can refer to the [pytest documentation][link_pytest] for more details on the failure. #### Changes to documentation From 415d272f17bdd559d16760beb02f7a28e2c0dfec Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Thu, 14 Nov 2019 17:17:54 -0500 Subject: [PATCH 30/59] Update docs/developing.rst Co-Authored-By: Ross Markello --- docs/developing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developing.rst b/docs/developing.rst index e26a9a8c1..81e07ce0d 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -30,7 +30,7 @@ You can do this with the following, which would make an archive ``my_data.tar.gz tar czf my_data.tar.gz my_data/*.nii.gz 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``. +``$TEDANADIR/tedana/tests/data/``, where ``TEDANADIR`` is your 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 From 302e17b0a5c989f77e0bd58a2b8cdb717a4cce62 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Thu, 14 Nov 2019 17:18:05 -0500 Subject: [PATCH 31/59] Update docs/developing.rst Co-Authored-By: Ross Markello --- docs/developing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developing.rst b/docs/developing.rst index 81e07ce0d..7d416111f 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -31,7 +31,7 @@ You can do this with the following, which would make an archive ``my_data.tar.gz Run the workflow with a known-working version, and put the outputs into a text file inside ``$TEDANADIR/tedana/tests/data/``, where ``TEDANADIR`` is your local ``tedana repository``. -You can follow the model our `five echo set`_, which has the following steps: +To write the test function you can follow the model of our `five echo set`_, which takes 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 From 1d7589653d252578c96b3d9ec795e0922b9e24e5 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Thu, 14 Nov 2019 17:18:15 -0500 Subject: [PATCH 32/59] Update docs/developing.rst Co-Authored-By: Ross Markello --- docs/developing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developing.rst b/docs/developing.rst index 7d416111f..b8f604f80 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -40,7 +40,7 @@ To write the test function you can follow the model of our `five echo set`_, whi actual output. 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. +it to `OSF`_ 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 From 5af570812b478ce2a85fb8b64ffe30d246790dff Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Thu, 14 Nov 2019 17:18:44 -0500 Subject: [PATCH 33/59] Update docs/developing.rst Co-Authored-By: Ross Markello --- docs/developing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developing.rst b/docs/developing.rst index b8f604f80..29bccdbb9 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -180,7 +180,7 @@ We run monthly developer calls via Zoom. You can see the schedule via the tedana `google calendar`_. Everyone is welcome. -We look forward to meeting you there :hibiscus: +We look forward to meeting you there! .. _git: https://git-scm.com/ .. _`git pro`: https://git-scm.com/book/en/v2 From e4fe82f0faa924e0c55a026e0d788c2675b0df7a Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Thu, 14 Nov 2019 17:19:43 -0500 Subject: [PATCH 34/59] Update CONTRIBUTING.md Co-Authored-By: Ross Markello --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e48b9c725..4becd276e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -251,7 +251,7 @@ 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): +### Pull Request Checklist (Before Requesting Review): - [ ] 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 From 157bc7ac3254952e2cae87045e87bd9678a59dc1 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Sun, 24 Nov 2019 21:09:10 -0500 Subject: [PATCH 35/59] Update CONTRIBUTING.md Co-Authored-By: Elizabeth DuPre --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4becd276e..e1d72ebaf 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -138,7 +138,7 @@ Once you've run this, your repository should be set for most changes (i.e., you 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 +Using a new branch allows you to follow the standard 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. From 52ab11badfbeb88c40ebdf864e763d2f1cfb237c Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Sun, 24 Nov 2019 21:10:02 -0500 Subject: [PATCH 36/59] Update CONTRIBUTING.md Co-Authored-By: Elizabeth DuPre --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e1d72ebaf..946f3ed91 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -185,7 +185,7 @@ 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. -When you open a Pull Request, CircleCI will run all tests. +Regardless, when you open a Pull Request, we use CircleCI to run all unit and integration tests. ``` # All tests; final checks before pushing From eecbf62a0c14be0f77c76d292e46dcdfa9367107 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Sun, 24 Nov 2019 21:12:06 -0500 Subject: [PATCH 37/59] Update CONTRIBUTING.md Co-Authored-By: Elizabeth DuPre --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 946f3ed91..374701ff8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -167,7 +167,7 @@ If you don't know what rebasing is, don't do it at all, as it is the easiest way 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 +If you are new to ``git`` and would like to work in a graphical user interface (GUI), 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] From eecd33b751b7041f6ec6eb8b0c72cd1b27161bd7 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 25 Nov 2019 15:21:01 -0500 Subject: [PATCH 38/59] Add @rmarkello suggested link --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 374701ff8..e3c731294 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -330,7 +330,7 @@ You're awesome. :wave::smiley: [writing_formatting_github]: https://help.github.com/articles/getting-started-with-writing-and-formatting-on-github [markdown]: https://daringfireball.net/projects/markdown [rick_roll]: https://www.youtube.com/watch?v=dQw4w9WgXcQ -[restructuredtext]: http://docutils.sourceforge.net/rst.html#user-documentation +[restructuredtext]: http://www.sphinx-doc.org/en/master/usage/restructuredtext/index.html [sphinx]: http://www.sphinx-doc.org/en/master/index.html [readthedocs]: https://docs.readthedocs.io/en/latest/index.html From 267bf66ff9f561b78b21ae5c40e57e07f869c93a Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 25 Nov 2019 15:22:46 -0500 Subject: [PATCH 39/59] Address @emdupre review --- CONTRIBUTING.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e3c731294..6830b59aa 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -181,7 +181,7 @@ flake8 $TEDANADIR/tedana ``` and unit/integration tests by running `pytest` (more details below). -If you know a file will test your change, you can run only that test. +If you know a file will test your change, you can run only that test (see "One test file only" below). Alternatively, running all unit tests is relatively quick and should be fairly comprehensive. Running all `pytest` tests will be useful for pre-pushing checks. @@ -192,7 +192,7 @@ Regardless, when you open a Pull Request, we use CircleCI to run all unit and in pytest $TEDANADIR/tedana/tests # Unit tests and linting only pytest --skipintegration $TEDANADIR/tedana/tests -# One test file in particular +# One test file only pytest $TEDANADIR/tedana/tests/test_file.py # Test one function in a file pytest -k my_function $TEDANADIR/tedana/tests/test_file.py From 6aa2296641c7ef9582e2a507f5b40dd7573f800d Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 25 Nov 2019 15:27:56 -0500 Subject: [PATCH 40/59] Offer alternative to Gitter --- CONTRIBUTING.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6830b59aa..4399aa37b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -251,7 +251,7 @@ 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): +### Pull Request Checklist (For Fastest Review): - [ ] 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 @@ -271,7 +271,8 @@ git checkout OTHERBRANCH git merge master ``` -In the event of confusion, please ping in the Gitter for help. +In the event of confusion, please open a draft pull request of what you're working on and indicate +the problem you're having so that a developer can help you. We strongly recommend you *not* use the `git rebase` command when working with and contributing to the `tedana` repository. ### Advanced Development From b7374a1405d080c2ff9e707302dfe14e7d5a99f3 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 25 Nov 2019 15:29:47 -0500 Subject: [PATCH 41/59] Address @emdupre review --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4399aa37b..8f59acc4a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -275,7 +275,7 @@ In the event of confusion, please open a draft pull request of what you're worki the problem you're having so that a developer can help you. We strongly recommend you *not* use the `git rebase` command when working with and contributing to the `tedana` repository. -### Advanced Development +### Comprehensive Developer Guide For additional, in-depth information on contributing to `tedana`, please see our Developing Guidelines on [readthedocs][link_developing_rtd]. ## Style Guide From 3ad460c96d25f08974ad5d585d0d87f7609a8bf1 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 25 Nov 2019 15:30:55 -0500 Subject: [PATCH 42/59] Update docs/developing.rst Co-Authored-By: Elizabeth DuPre --- docs/developing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developing.rst b/docs/developing.rst index 29bccdbb9..c01bff195 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -15,7 +15,7 @@ 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. +For unit tests, we try to keep tests from 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 From 13e3daa280f853af33cae1687e8521c009dcd5c4 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 25 Nov 2019 15:40:33 -0500 Subject: [PATCH 43/59] Outlines steps for integration tests more clearly --- docs/developing.rst | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/docs/developing.rst b/docs/developing.rst index 29bccdbb9..2ee8d1832 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -15,13 +15,27 @@ 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. +Unit Tests +---------- 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 only the files you'd like to +Integration Tests +----------------- +Adding integration tests is relatively rare. +An integration test will be a complete multi-echo dataset called with some set of options to ensure +end-to-end pipeline functionality. +These tests are relatively computationally expensive but aid us in making sure the pipeline is +stable during large sets of changes. +If you believe you have a dataset that will test ``tedana`` more completely, please open an issue +before attempting to add an integration test. +After securing the appropriate permission from the dataset owner to share it with ``tedana``, you +can use the following procedure: + +(1) Make a ``tar.gz`` file which will unzip to be only the files you'd like to run a workflow on. You can do this with the following, which would make an archive ``my_data.tar.gz``: @@ -29,8 +43,10 @@ You can do this with the following, which would make an archive ``my_data.tar.gz tar czf my_data.tar.gz my_data/*.nii.gz -Run the workflow with a known-working version, and put the outputs into a text file inside +(2) Run the workflow with a known-working version, and put the outputs into a text file inside ``$TEDANADIR/tedana/tests/data/``, where ``TEDANADIR`` is your local ``tedana repository``. + +(3) Write a test function in ``test_integration.py``. To write the test function you can follow the model of our `five echo set`_, which takes the following steps: 1. Check if a pytest user is skipping integration, skip if so @@ -39,9 +55,10 @@ To write the test function you can follow the model of our `five echo set`_, whi #. Use ``resources_filename`` and ``check_integration_outputs`` to compare your expected output to actual output. -If you need to upload new data, you will need to contact the maintainers and ask them to either add +(4) If you need to upload new data, you will need to contact the maintainers and ask them to either add it to `OSF`_ 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 + +(5) 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 @@ -53,7 +70,8 @@ and on an indented line below put with ``TEST`` your test function's name. 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 + +(6) 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 From c720db15b8e52ab13fb8a957fa0f428c0f986187 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 25 Nov 2019 15:43:55 -0500 Subject: [PATCH 44/59] Address @emdupre review --- docs/developing.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/developing.rst b/docs/developing.rst index 52fdeef98..b31b00a8e 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -74,6 +74,8 @@ test in addition. (6) 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``. +Viewing CircleCI Outputs +------------------------ 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_) From 7102ff0b2cb1b5b1d6a0755085f3600601cf00f0 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Mon, 25 Nov 2019 15:45:30 -0500 Subject: [PATCH 45/59] Moves monthly call to top of docs --- docs/developing.rst | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/docs/developing.rst b/docs/developing.rst index b31b00a8e..f95757d60 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -7,6 +7,16 @@ This webpage is intended to guide users through making making changes to The worked example also offers some guidelines on approaching testing when adding new functions. + +Monthly Developer Calls +======================= +We run monthly developer calls via Zoom. +You can see the schedule via the tedana `google calendar`_. + +Everyone is welcome. +We look forward to meeting you there! + + Adding and Modifying Tests ========================== Testing is an important component of development. @@ -194,13 +204,6 @@ We should then do the following cleanup with our git repository: and we're good to go! -Monthly Developer Calls -======================= -We run monthly developer calls via Zoom. -You can see the schedule via the tedana `google calendar`_. - -Everyone is welcome. -We look forward to meeting you there! .. _git: https://git-scm.com/ .. _`git pro`: https://git-scm.com/book/en/v2 From 6753c82416ef12b89d0184b927428913654a54f9 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Tue, 3 Dec 2019 14:51:06 -0500 Subject: [PATCH 46/59] Rewords CONTRIBUTING to be more encouraging --- CONTRIBUTING.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8f59acc4a..53e87fda0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -136,10 +136,9 @@ 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. +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 GitHub -workflow when making changes. +Using a new branch allows you to follow the standard 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. @@ -182,8 +181,7 @@ flake8 $TEDANADIR/tedana and unit/integration tests by running `pytest` (more details below). If you know a file will test your change, you can run only that test (see "One test file only" below). -Alternatively, running all unit tests is relatively quick and should be -fairly comprehensive. +Alternatively, running all unit tests is relatively quick and should be fairly comprehensive. Running all `pytest` tests will be useful for pre-pushing checks. Regardless, when you open a Pull Request, we use CircleCI to run all unit and integration tests. @@ -247,9 +245,12 @@ Pull requests should be submitted early and often! 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. -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. +If no comments or commits occur on an open Pull Request, stale-bot will comment in order to remind +both you and the maintainers that the pull request is open. +If at this time you are awaiting a developer response, please ping them to remind them. +If you are no longer interested in working on the pull request, let us know and we will ask to +continue working on your branch. +Thanks for contributing! ### Pull Request Checklist (For Fastest Review): - [ ] Check that all tests are passing ("All tests passsed") From dbc6d6e55c04cf7e6c8e4370317b2b6d8dda9484 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Tue, 3 Dec 2019 14:54:25 -0500 Subject: [PATCH 47/59] Adds filenaming convention for outputs files --- docs/developing.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/developing.rst b/docs/developing.rst index f95757d60..9064a37d6 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -55,6 +55,8 @@ You can do this with the following, which would make an archive ``my_data.tar.gz (2) Run the workflow with a known-working version, and put the outputs into a text file inside ``$TEDANADIR/tedana/tests/data/``, where ``TEDANADIR`` is your local ``tedana repository``. +We encourage using the convention ``__echo_outputs.txt``, appending ``verbose`` +to the filename if the integration test uses ``tedana`` in the verbose mode. (3) Write a test function in ``test_integration.py``. To write the test function you can follow the model of our `five echo set`_, which takes the following steps: From 1bd9b198c292571c9ac323e4130a7b3222b989d8 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Tue, 3 Dec 2019 15:11:08 -0500 Subject: [PATCH 48/59] Ask not to rewrite history --- CONTRIBUTING.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 53e87fda0..13522581d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -226,6 +226,9 @@ git push -u origin MYBRANCH and GitHub will respond by giving you a link to open a pull request to ME-ICA/tedana. +Once you have pushed changes to the repository, please do not use commands such as rebase and +amend, as they will rewrite your history and make it difficult for developers to work with you on +your pull request. You can read more about that [here][link_git_rewriting]. 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: @@ -274,7 +277,6 @@ git merge master In the event of confusion, please open a draft pull request of what you're working on and indicate the problem you're having so that a developer can help you. -We strongly recommend you *not* use the `git rebase` command when working with and contributing to the `tedana` repository. ### Comprehensive Developer Guide For additional, in-depth information on contributing to `tedana`, please see our Developing Guidelines on [readthedocs][link_developing_rtd]. @@ -375,3 +377,4 @@ You're awesome. :wave::smiley: [link_git_kraken]: https://www.gitkraken.com/ [link_github_desktop]: https://desktop.github.com/ [link_source_tree]: https://desktop.github.com/ +[link_git_rewriting]: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History From 51142ef2780820d1f0989e2e0d83a1256f9b33f0 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Fri, 6 Dec 2019 09:38:50 -0500 Subject: [PATCH 49/59] Removes rebase reference --- CONTRIBUTING.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 13522581d..6c81028e2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -161,9 +161,6 @@ 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`` and would like to work in a graphical user interface (GUI), there are several GUI git clients that you may find helpful, such as From 244f86547f11bbdfe509b326d66439bcaf6d76b9 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Fri, 6 Dec 2019 09:39:09 -0500 Subject: [PATCH 50/59] Splits sentences off into rich text --- CONTRIBUTING.md | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6c81028e2..fdc515f92 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -182,14 +182,20 @@ Alternatively, running all unit tests is relatively quick and should be fairly c Running all `pytest` tests will be useful for pre-pushing checks. Regardless, when you open a Pull Request, we use CircleCI to run all unit and integration tests. +All tests; final checks before pushing ``` -# All tests; final checks before pushing pytest $TEDANADIR/tedana/tests -# Unit tests and linting only +``` +Unit tests and linting only +``` pytest --skipintegration $TEDANADIR/tedana/tests -# One test file only +``` +One test file only +``` pytest $TEDANADIR/tedana/tests/test_file.py -# Test one function in a file +``` +Test one function in a file +``` pytest -k my_function $TEDANADIR/tedana/tests/test_file.py ``` From ee3ea7c7997a84a80fb7ba753dc10238d1a46acd Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Fri, 6 Dec 2019 09:42:29 -0500 Subject: [PATCH 51/59] Remove 'After changes are merged' section --- CONTRIBUTING.md | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fdc515f92..755508bb3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -265,22 +265,6 @@ Thanks for contributing! - [ ] 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, -which you can do via - -``` -git checkout master -git fetch upstream master -git merge upstream/master -git checkout OTHERBRANCH -git merge master -``` - -In the event of confusion, please open a draft pull request of what you're working on and indicate -the problem you're having so that a developer can help you. - ### Comprehensive Developer Guide For additional, in-depth information on contributing to `tedana`, please see our Developing Guidelines on [readthedocs][link_developing_rtd]. From 35f0a4412405f10b281a7b5628495f31e5a4425e Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Fri, 6 Dec 2019 09:47:03 -0500 Subject: [PATCH 52/59] Remove fetch and merge for active branch --- CONTRIBUTING.md | 7 ------- 1 file changed, 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 755508bb3..28c0dd199 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -154,13 +154,6 @@ 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 -``` - Please make sure to review the `tedana` [style conventions](#style-guide) and test your changes. If you are new to ``git`` and would like to work in a graphical user interface (GUI), there are several GUI git clients that you may find helpful, such as From dd1791f066d8116ef64c19c837101d83b82dbe95 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Fri, 6 Dec 2019 11:17:26 -0500 Subject: [PATCH 53/59] Update docs/developing.rst Co-Authored-By: Ross Markello --- docs/developing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developing.rst b/docs/developing.rst index 9064a37d6..03e40fb30 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -146,7 +146,7 @@ We merge any changes from the upstream master branch into our branch via .. code-block:: bash - git checkout feature/say_hello # unless you're already there + git checkout feature/say_hello git fetch upstream master git merge upstream/master From 88d176d3044ff1998e5172bff82661638bea1524 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Thu, 12 Dec 2019 14:02:10 -0500 Subject: [PATCH 54/59] Adds two reviewer rule --- CONTRIBUTING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 28c0dd199..949fe5d83 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -210,6 +210,7 @@ from the `docs` directory in your local `tedana` repository. You should then be When opening the pull request, we ask that you follow some [specific conventions](#pull-requests). We outline these below. 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. +When you have two approving reviewers and all tests are passing, your pull request may be merged. ### Pull Requests From 8c68d892ca44a0692d2b1c3da3030f79208acc9e Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Fri, 13 Dec 2019 14:33:57 -0500 Subject: [PATCH 55/59] Update CONTRIBUTING.md --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 949fe5d83..6fbe3f7d7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -140,7 +140,7 @@ 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 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. +Before making a new branch, make sure your master is up to date with the following commands: ``` git checkout master From 7491b92341622538e9fc7c64b9b3f5d98fd20f56 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Fri, 13 Dec 2019 14:45:33 -0500 Subject: [PATCH 56/59] Adds link to contributing --- docs/developing.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/developing.rst b/docs/developing.rst index 03e40fb30..32900cbb4 100644 --- a/docs/developing.rst +++ b/docs/developing.rst @@ -6,6 +6,7 @@ This webpage is intended to guide users through making making changes to ``tedana``'s codebase, in particular working with tests. The worked example also offers some guidelines on approaching testing when adding new functions. +Please check out our `contributing guide`_ for getting started. Monthly Developer Calls @@ -222,3 +223,4 @@ and we're good to go! .. _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 .. _`google calendar`: https://calendar.google.com/calendar/embed?src=pl6vb4t9fck3k6mdo2mok53iss%40group.calendar.google.com +.. _`contributing guide`: https://github.com/ME-ICA/tedana/blob/master/CONTRIBUTING.md From 0cc2369ab59b824f523fdd3e88561c394521e0ae Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Fri, 13 Dec 2019 15:05:33 -0500 Subject: [PATCH 57/59] Address review --- CONTRIBUTING.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 949fe5d83..2dac15c1b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,9 +14,13 @@ Already know what you're looking for in this guide? Jump to the following sectio * [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) +* [Installing in editable mode](#Run-the-developer-setup) * [Making a change](#making-a-change) +* [Testing your change](#Test-your-changes) +* [Viewing Documentation Locally](#Changes-to-documentation) * [Structuring contributions](#style-guide) * [Recognizing contributors](#recognizing-contributions) +* [Monthly calls and testing guidelines][link_developing_rtd] Don't know where to get started? Read [Joining the conversation](#joining-the-conversation) and pop into From 28368a8c4a51a842117c4ca0c8a109e5f9100e1e Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Fri, 13 Dec 2019 15:08:53 -0500 Subject: [PATCH 58/59] Try to fix links --- CONTRIBUTING.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b3cb3e578..49fd17f01 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,9 +14,9 @@ Already know what you're looking for in this guide? Jump to the following sectio * [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) -* [Installing in editable mode](#Run-the-developer-setup) +* [Installing in editable mode](#3-Run-the-developer-setup) * [Making a change](#making-a-change) -* [Testing your change](#Test-your-changes) +* [Testing your change](#5-Test-your-changes) * [Viewing Documentation Locally](#Changes-to-documentation) * [Structuring contributions](#style-guide) * [Recognizing contributors](#recognizing-contributions) From f9a7295de24a413adac41d7fc68cda6d6c6f0640 Mon Sep 17 00:00:00 2001 From: Joshua Teves Date: Fri, 13 Dec 2019 15:13:10 -0500 Subject: [PATCH 59/59] Fix more links --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 49fd17f01..3116ab1ac 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -19,7 +19,7 @@ Already know what you're looking for in this guide? Jump to the following sectio * [Testing your change](#5-Test-your-changes) * [Viewing Documentation Locally](#Changes-to-documentation) * [Structuring contributions](#style-guide) -* [Recognizing contributors](#recognizing-contributions) +* [Recognizing contributors](#Recognizing-contributors) * [Monthly calls and testing guidelines][link_developing_rtd] Don't know where to get started?