|
| 1 | +Contributing |
| 2 | +============ |
| 3 | + |
| 4 | +We actively encourage contributions and collaboration in the development of rydiqule. |
| 5 | +Unforunately, rydiqule's development is done privately, *for reasons*. |
| 6 | +If you would like to submit a PR for all but the most trivial of changes, |
| 7 | +please e-mail us directly (david.h.meyer3.civ@army.mil or kevin.c.cox29.civ@army.mil) |
| 8 | +so we can discuss the collaboration. |
| 9 | + |
| 10 | +Tips for a successful contribution |
| 11 | +++++++++++++++++++++++++++++++++++ |
| 12 | + |
| 13 | +If you would like to submit a pull request that improves or fixes rydiqule, |
| 14 | +first off, thank you! |
| 15 | +A wider set of contributors to a project significantly improves the quality |
| 16 | +of the project as well as its pace of development. |
| 17 | + |
| 18 | +Secondly, because of limitations placed on rydiqule's primary developers, |
| 19 | +development deviates from a typical open-source project in that day-to-day work is done privately, |
| 20 | +with bulk public code releases. |
| 21 | +The most important difference for a potential contributor is that early communication |
| 22 | +with the core developers is even more important so we can advise how to move forward. |
| 23 | + |
| 24 | +Finally, writing code, especially a PR to someone else's project, |
| 25 | +is similar to writing a journal article for peer-review. |
| 26 | +The (code) reviewer needs to understand what you have done and why. |
| 27 | +Ultimately you need to *persuade* them to accept. |
| 28 | +Below is a general list of tips for producing a PR that can be merged quickly. |
| 29 | + |
| 30 | +Ask *before* doing a lot of work |
| 31 | +-------------------------------- |
| 32 | + |
| 33 | +Unlike paper reviews, the code review process is not blind. |
| 34 | +As such, and as is common to any open-source project, it is good form to reach out to the developers |
| 35 | +*before* submitting a PR with significant changes, via an issue on github or a direct message to the devs. |
| 36 | +This can save you a lot of time as it allows the devs |
| 37 | +to provide high-level guidance on what approach is likely to work and/or be accepted. |
| 38 | +It avoids duplicated effort if we are already working on a solution. |
| 39 | +It avoids wasted effort on things we have already rejected as not viable. |
| 40 | +It also provides an opportunity for us to directly collaborate and help. |
| 41 | +Finally, it allows us to set expectations on scope, |
| 42 | +such as clarifying when a unit test isn't necessary. |
| 43 | + |
| 44 | +Make things digestible |
| 45 | +---------------------- |
| 46 | + |
| 47 | +In much the same way a paper with multiple pages of non-stop equations can be hard to follow, |
| 48 | +submitting a PR with 1000s of changed lines of code, especially without having reached out first, |
| 49 | +makes the PR very difficult to digest. |
| 50 | +And we will not accept code we do not understand. |
| 51 | + |
| 52 | +The key to digestible PRs is breaking changes down into small, logical, digestible steps. |
| 53 | + |
| 54 | +- PRs are most like sections of a paper. |
| 55 | + Ensure that each PR addresses a single task. |
| 56 | + For example, don't implement unrelated features in a single PR, |
| 57 | + or undertake large refactors alongside a new feature. |
| 58 | + It is OK (even expected) to have a PR depend on another to be merged first. |
| 59 | +- Commits are most like paragraphs in a paper. |
| 60 | + You should tend to use many small commits within a PR, |
| 61 | + ensuring each commit has a single purpose and a sufficiently detailed commit message. |
| 62 | + For example, moving a file and editing its contents should be separate commits. |
| 63 | + Updating many inter-related type hints should be a single commit. |
| 64 | +- Careful application of git to edit commit history to organize edits after the fact |
| 65 | + should be considered. |
| 66 | + After all, it is rare the the order of discovery matches the best order for communicating the result. |
| 67 | + But note that editing history *after* creating the PR leads to annoyance for |
| 68 | + others who have already checked out your changes. |
| 69 | +- Finally, don't overthink it. |
| 70 | + If the idea can be communicated in a concise single page, |
| 71 | + it is generally best to keep it that way instead of fluffing out the manuscript. |
| 72 | + The same goes for PRs: learn to recognize time-wasting bloat and low-value perfection chasing. |
| 73 | + If the change is straight-forward, don't waste time checking every box or revising commit messages. |
| 74 | + |
| 75 | +Follow style guides |
| 76 | +------------------- |
| 77 | + |
| 78 | +Submitting a PR that does not follow standard conventions |
| 79 | +makes it much harder for the reviewer to follow. |
| 80 | +Even if the underlying work is fundamentally correct, |
| 81 | +the differences in (ultimately arbitrary) styles consumes the bulk of the review effort. |
| 82 | + |
| 83 | +Please ensure that your code follows the general style guides and practices for rydiqule. |
| 84 | +This ensures code review focuses on important things, rather than minor details like formatting etc. |
| 85 | +Aspects include :doc:`code linting <linting>`, :doc:`docstrings and higher-level docs <docs>`, |
| 86 | +and :doc:`type hinting <types>`. |
| 87 | + |
| 88 | +Unit tests and examples |
| 89 | +----------------------- |
| 90 | + |
| 91 | +Submitting a PR that lacks context or is missing supporting justifications |
| 92 | +is much akin to submitting an incomplete manuscript. |
| 93 | +Even if the work represents a significant advancement, |
| 94 | +the missing details make it hard for the reviewer to understand and therefore accept. |
| 95 | + |
| 96 | +For code, these supporting elements are provided by clear tests and examples, |
| 97 | +especially in the associated github issue or pull request descriptions. |
| 98 | +We also expect unit tests and examples to be included into the repository, where appropriate. |
| 99 | +Please consult :doc:`our unit test documentation <tests>` to see what conventions we use. |
| 100 | +Example notebooks should largely follow the style of existing example notebooks in the documentation. |
| 101 | +Those notebooks live in the `/docs/source/examples <https://github.com/QTC-UMD/rydiqule/tree/main/docs/source/examples>`_ |
| 102 | +and `/docs/source/intro_nbs <https://github.com/QTC-UMD/rydiqule/tree/main/docs/source/intro_nbs>`_ directories of the repository. |
| 103 | + |
| 104 | +If you are fixing a bug, unless it is very trivial, |
| 105 | +we will expect a unit test to accompany the fix that demonstrates the issue and proves the fix works. |
| 106 | +Ideally, the unit test should be the first commit so it can easily be shown what the issue is. |
| 107 | + |
| 108 | +If you are adding a new feature, appropriate unit testing demonstrating the new feature works will be expected. |
| 109 | +Depending on the scope of the enhancement, an example added to an existing notebook |
| 110 | +or more likely a new example notebook will also be expected. |
| 111 | + |
| 112 | +Please be patient |
| 113 | +----------------- |
| 114 | + |
| 115 | +We try very hard to outpace Physical Review timelines. |
| 116 | +However, like most open-source project developers/maintainers, |
| 117 | +this is not our primary responsibility. |
| 118 | +While we strive to at least acknowledge messages quickly, |
| 119 | +we are busy and it may take time to respond. |
| 120 | +If things seem stuck, don't be afraid to ping again. |
| 121 | + |
| 122 | +We also strive to be direct in our communication. |
| 123 | +Especially in code reviews, this can come across as impatient or unappreciative. |
| 124 | +That is not our intention and we request you not take it personally. |
| 125 | +We strive to hold code released as a part of rydiqule to a high standard, |
| 126 | +as it is a general tool with a wide variety of users and use cases. |
| 127 | +We have to ensure contributions don't have unintended consequences for others, |
| 128 | +and that they are maintainable *by us* after you have moved on. |
| 129 | + |
| 130 | +Please be prepared for any PR to have many comments, questions, and requested changes before being merged. |
| 131 | +Niche enhancements that break usability for other applications are unlikely to be merged. |
| 132 | +While such contributions are important for science, |
| 133 | +we will likely direct them elsewhere (i.e. a fork, paper supplemental material). |
0 commit comments