-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DOC] Adds developer guideline start #446
Merged
jbteves
merged 65 commits into
ME-ICA:master
from
jbteves:feature/add_developer_guidelines
Dec 14, 2019
Merged
Changes from all commits
Commits
Show all changes
65 commits
Select commit
Hold shift + click to select a range
111cd6b
Adds developer guideline start
jbteves 7891407
Modifies CONTRIBUTING
jbteves 36cf608
Fixes dead link, removes project board
jbteves d1d945c
Adds more thorough developing guidelines
jbteves 18a2e6b
Fix lack of linting
jbteves ff70cac
Updates dev requirements
jbteves e7e542e
Adds flake8 instructions
jbteves ae88853
Adds instructions for retrieving CircleCI artifacts
jbteves 090f786
Updates config with correct CI version
jbteves f5cc4d7
Address @emdupre review
jbteves 74b7462
Addresses a different @emdupre review
jbteves 079a41a
Update CONTRIBUTING.md
jbteves 79e7b52
More addressing
jbteves 4b7f751
Merge remote-tracking branch 'origin/feature/add_developer_guidelines…
jbteves 94799a7
More review addressing
jbteves 6a3043f
Update CONTRIBUTING.md
jbteves db327ff
Update CONTRIBUTING.md
jbteves 558b74d
Update CONTRIBUTING.md
jbteves 30d125a
Update docs/developing.rst
jbteves 95921d4
Update CONTRIBUTING.md
jbteves a535ee9
Adds manual selection of artifiacts through CircleCI UI
jbteves d722317
Merge branch 'feature/add_developer_guidelines' of github.com:jbteves…
jbteves a63ccfe
Adds tar command
jbteves aa41ffe
Adds tar command
jbteves 8beeecd
Fix formatting
jbteves aa621c9
Adds example
jbteves a03f095
Stupid line break typo
jbteves 93c465b
Clarify some pytest stuff
jbteves 9ad1eca
Address @rmarkello review
jbteves 1c15264
Addresss @rmarkello review
jbteves d1af26b
Update CONTRIBUTING.md
jbteves 415d272
Update docs/developing.rst
jbteves 302e17b
Update docs/developing.rst
jbteves 1d75896
Update docs/developing.rst
jbteves 5af5708
Update docs/developing.rst
jbteves e4fe82f
Update CONTRIBUTING.md
jbteves 886ce30
Merge remote-tracking branch 'upstream/master' into feature/add_devel…
jbteves 157bc7a
Update CONTRIBUTING.md
jbteves 52ab11b
Update CONTRIBUTING.md
jbteves eecbf62
Update CONTRIBUTING.md
jbteves eecd33b
Add @rmarkello suggested link
jbteves 267bf66
Address @emdupre review
jbteves 6aa2296
Offer alternative to Gitter
jbteves b7374a1
Address @emdupre review
jbteves 3ad460c
Update docs/developing.rst
jbteves 13e3daa
Outlines steps for integration tests more clearly
jbteves 6a6a71f
Resolve conflict
jbteves c720db1
Address @emdupre review
jbteves 7102ff0
Moves monthly call to top of docs
jbteves 6753c82
Rewords CONTRIBUTING to be more encouraging
jbteves dbc6d6e
Adds filenaming convention for outputs files
jbteves 1bd9b19
Ask not to rewrite history
jbteves 51142ef
Removes rebase reference
jbteves 244f865
Splits sentences off into rich text
jbteves ee3ea7c
Remove 'After changes are merged' section
jbteves 35f0a44
Remove fetch and merge for active branch
jbteves dd1791f
Update docs/developing.rst
jbteves 8eab7e1
Merge branch 'feature/add_developer_guidelines' of github.com:jbteves…
jbteves 88d176d
Adds two reviewer rule
jbteves 8c68d89
Update CONTRIBUTING.md
jbteves 7491b92
Adds link to contributing
jbteves 0cc2369
Address review
jbteves cd4b709
Merge remote-tracking branch 'origin/feature/add_developer_guidelines…
jbteves 28368a8
Try to fix links
jbteves f9a7295
Fix more links
jbteves File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,13 +11,16 @@ 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) | ||
* [Installing in editable mode](#3-Run-the-developer-setup) | ||
* [Making a change](#making-a-change) | ||
* [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? | ||
Read [Joining the conversation](#joining-the-conversation) and pop into | ||
|
@@ -33,15 +36,6 @@ We also maintain a [gitter chat room][link_gitter] for more informal conversatio | |
There is significant cross-talk between these two spaces, and we look forward to hearing from you in either venue! | ||
As a reminder, we expect all contributions to `tedana` to adhere to our [code of conduct][link_coc]. | ||
|
||
### Monthly developer calls | ||
|
||
We run monthly developer calls via Zoom. | ||
You can see the schedule via the `tedana` [google calendar](https://calendar.google.com/calendar/embed?src=pl6vb4t9fck3k6mdo2mok53iss%40group.calendar.google.com). | ||
An agenda will be circulated in the gitter channel in advance of the meeting. | ||
|
||
Everyone is welcome. | ||
We look forward to meeting you there :hibiscus: | ||
|
||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
## Contributing small documentation changes | ||
If you are new to GitHub and just have a small documentation change | ||
recommendation, please submit it to [our e-mail address](mailto:[email protected]) | ||
|
@@ -92,11 +86,6 @@ towards ``tedana``'s shared vision. | |
We might have just missed it, or we might not (yet) see how it aligns with the overall project structure. | ||
These conversations are important to have, and we are excited to hear your perspective! | ||
|
||
* The **project board** is an automated [Kanban board][link_kanban] to keep track of what is currently underway | ||
(in progress), what has been completed (done), and what remains to be done for a specific release. | ||
The ``tedana`` maintainers use this board to keep an eye on how tasks are progressing week by week. | ||
|
||
|
||
### Issue labels | ||
|
||
The current list of labels are [here][link_labels] and include: | ||
|
@@ -151,30 +140,66 @@ 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 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 with the following commands: | ||
|
||
Using a new branch allows you to follow the standard "fork/branch/commit/pull-request/merge" GitHub workflow when making changes. [This guide][link_gitworkflow] provides a useful overview for this workflow. | ||
``` | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
git checkout master | ||
git fetch upstream master | ||
git merge upstream/master | ||
``` | ||
|
||
Then, make your new branch. | ||
|
||
``` | ||
git checkout -b MYBRANCH | ||
``` | ||
|
||
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 | ||
- [GitKraken][link_git_kraken] | ||
- [GitHub Desktop][link_github_desktop] | ||
- [SourceTree][link_source_tree] | ||
|
||
Before creating your pull request, please make sure to review the `tedana` [style conventions](#style-guide). | ||
|
||
### 5. Test your changes | ||
emdupre marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#### Changes to code | ||
You can run style checks by running the following: | ||
``` | ||
flake8 $TEDANADIR/tedana | ||
``` | ||
|
||
For changes to the codebase, we suggest using our development Docker container which will run all the necessary checks and tests to ensure your code is ready to be merged into `tedana`! | ||
(This does require that you have a local install of [Docker](https://www.docker.com/products/docker-desktop).) | ||
You can run all the checks with: | ||
and unit/integration tests by running `pytest` (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. | ||
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 | ||
``` | ||
pytest $TEDANADIR/tedana/tests | ||
``` | ||
Unit tests and linting only | ||
``` | ||
docker run --tty --rm -v ${PWD}:/tedana tedana/tedana-dev:latest run_all_tests | ||
pytest --skipintegration $TEDANADIR/tedana/tests | ||
``` | ||
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 within your local `tedana` repository. | ||
(**N.B.** It is possible that, depending on your Docker setup, you may need to increase the amount of memory available to Docker in order to run the `tedana` test suite. | ||
You can either do this permanently by editing your Docker settings or temporarily by adding `--memory=4g` to the above `docker run` command.) | ||
|
||
This will print out a number of different status update messages as the tests run, but if you see `"FINISHED RUNNING ALL TESTS! GREAT SUCCESS"` then it means everything finished succesfully. | ||
If not, there should be some helpful outputs that specify which tests failed. | ||
The test run will indicate the number of passes and failures. | ||
jbteves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Most often, the failures 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 | ||
|
||
|
@@ -189,11 +214,23 @@ 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. | ||
|
||
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. | ||
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: | ||
|
||
|
@@ -202,20 +239,32 @@ 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. | ||
|
||
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]**. | ||
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") | ||
- [ ] 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) | ||
|
||
### Comprehensive Developer Guide | ||
For additional, in-depth information on contributing to `tedana`, please see our Developing Guidelines on [readthedocs][link_developing_rtd]. | ||
|
||
## Style Guide | ||
|
||
|
@@ -270,7 +319,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 | ||
|
||
|
@@ -289,6 +338,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 +356,10 @@ You're awesome. :wave::smiley: | |
[link_all-contributors-bot]: https://allcontributors.org/docs/en/bot/overview | ||
[link_all-contributors-bot-usage]: https://allcontributors.org/docs/en/bot/usage | ||
[link_stemmrolemodels]: https://github.com/KirstieJane/STEMMRoleModels | ||
[link_pytest]: https://docs.pytest.org/en/latest/usage.html | ||
[link_developing_rtd]: https://tedana.readthedocs.io/en/latest/developing.html | ||
|
||
[link_git_kraken]: https://www.gitkraken.com/ | ||
[link_github_desktop]: https://desktop.github.com/ | ||
[link_source_tree]: https://desktop.github.com/ | ||
[link_git_rewriting]: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,3 +8,4 @@ numpydoc | |
pytest | ||
pytest-cov | ||
requests | ||
sphinx |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very useful section, I think there are a couple of other points in the document readers might want to quickly navigate to, so I would add:
I would also link to the developer docs here with a brief description of the contents, say:
See also our Developing Guidelines on [readthedocs][link_developing_rtd] for additional information, particularly on our monthly developer calls and writing tests for tedana.