-
Notifications
You must be signed in to change notification settings - Fork 17
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
Style Guide #19
Merged
bourque
merged 10 commits into
IMAP-Science-Operations-Center:dev
from
bourque:style-guide
Jul 14, 2023
Merged
Style Guide #19
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
47d62d9
Initial commit of style guide
bourque 8cef76a
Reordered some sections, added some recommended tools/libraries
bourque 1371063
Some clarifications on the git workflow
bourque ccef051
Fixed some broken links; addressed some review comments
bourque 64585b1
Added poetry section, added tl;dr section, more consistent use of mar…
bourque ed73bf9
Added release workflow section, reformatted to use 120 characters ins…
bourque 7cdb578
Added section on updating poetry environments
maxinelasp 6dcb24b
Added checklist for contributors and reviewers of PRs
bourque d806536
Changes from review comments. Thank you @maxinelasp, @greglucas, @tec…
bourque 8010dcd
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,206 @@ | ||
`imap_processing` Style Guide | ||
============================= | ||
|
||
This document serves as a style guide for all `imap_processing` software | ||
development. Any potential contribution to the `imap_processing` repository | ||
should be checked against this guide, and any violation of the guide should be | ||
fixed before the code is committed to the `main` or `dev` branches. | ||
|
||
|
||
## tl;dr | ||
|
||
The following is a short version of this style guide to be used as a quick | ||
reference. Further details about each of these are provided in the guide. | ||
|
||
1. Use a [forking workflow](#git-&-github-workflow) for git/GitHub contributions. | ||
2. Update the [`poetry` environment](#poetry-environment) when dependencies | ||
change. | ||
3. Use PEP8 for [python coding](#python-coding), with a few exceptions. | ||
4. Use PEP257 and `numpydocs` for [docstring conventions](#api-documentation), | ||
with a few exceptions. | ||
5. Use nominal semantic versioning for [version numbers](#versioning). | ||
6. Be mindful of committing credentials and other [sensitive information](#security). | ||
7. Use specific [tools and libraries](#tools-and-library-recommendations) when | ||
able to. | ||
|
||
|
||
## git & GitHub Workflow | ||
|
||
The best method for contributing software to the `imap_processing` repository is | ||
a workflow that involves forking the repository, developing changes on "feature" | ||
branches, and opening pull requests through GitHub. | ||
|
||
The following diagram depicts this workflow (credit to | ||
[Atlassian](https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow)): | ||
|
||
<img src="https://wac-cdn.atlassian.com/dam/jcr:cc0b526e-adb7-4d45-874e-9bcea9898b4a/04%20Hotfix%20branches.svg?cdnVersion=1089" alt="git and GitHub Workflow" width="500" height="500"> | ||
|
||
As such, all feature branches should be branched off of and merged back into the | ||
`dev` branch. | ||
|
||
### Contributing new features | ||
|
||
*Note: Steps (1) through (4) only to be done once.* | ||
bourque marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
1. Create a personal fork of the `imap_processing` repository by visiting the | ||
repository main `IMAP-Science-Operations-Center` `imap_processing` | ||
[repository](https://github.com/IMAP-Science-Operations-Center/imap_processing) | ||
and clicking the `Fork` button near the top-right of the page. Follow the | ||
various steps to create a fork under your GitHub account. | ||
2. Make a local copy of your personal fork by cloning the repository, using the | ||
URL found by clicking the green "clone" button. | ||
3. Ensure that the personal fork is pointing to the `upstream` `imap_processing` | ||
repository: | ||
|
||
git remote add upstream https://github.com/IMAP-Science-Operations-Center/imap_processing.git # for HTTPS | ||
git remote add upstream git@github.com:IMAP-Science-Operations-Center/imap_processing.git # for SSH | ||
|
||
4. Retrieve the `upstream` `dev` branch: | ||
|
||
git fetch upstream | ||
git checkout -b dev upstream/dev | ||
|
||
6. Create a feature branch off of the `dev` branch to develop changes on. | ||
Branch names should be short but descriptive (e.g. | ||
`update-codice-unit-tests`) and not too generic (e.g. `bug-fix`, `updates`). | ||
Consistent use of hyphens is encouraged. | ||
|
||
git checkout -b <branchname> | ||
|
||
7. Make changes to the branch using the nominal `git add`/`git commit` cycle: | ||
|
||
git add <new or changed files you want to commit> | ||
git commit -m 'Explaination of the changes' | ||
|
||
8. Push the feature branch to your personal fork's GitHub repository: | ||
|
||
git push origin <branchname> | ||
|
||
9. On the `IMAP-Science-Operations-Center` `imap_processing` | ||
[repository](https://github.com/IMAP-Science-Operations-Center/imap_processing) | ||
create a new pull request. Click on the "compare across forks" link to | ||
enable the pull request to use your fork. Set the "base repository" to | ||
`IMAP-Science-Operations-Center` and "base" to `dev`. Set the "head | ||
repository" to the `imap_processing` repository under your personal fork | ||
and "compare" to your feature branch. If the feature branch is still under | ||
development, you can click the "Convert to draft" button under the | ||
"Reviewers" section, or add a "[WIP]" at the beginning of the pull request | ||
title to signify that the pull request is not ready to be merged. | ||
|
||
10. Assign at least one reviewer to the pull request. They will review your | ||
bourque marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pull request and either accept the request or ask for additional changes. | ||
If additional changes are needed, iterate through steps (6) and (7) until | ||
you and the reviewer(s) are satisfied. | ||
|
||
11. Once the pull request has been accepted, you can merge the pull request and | ||
delete the feature branch. | ||
|
||
|
||
### Making hotfixes | ||
|
||
As shown in the diagram above, sometimes hotfixes need to be made directly to | ||
the `main` branch. The workflow for this scenario is as follows: | ||
|
||
1. Assuming steps (1) through (3) in the previous section are already completed, | ||
create a new branch named `hotfix-<description>` off of the `main` branch, | ||
and commit any necessary changes following the nominal `git add`/`git commit` | ||
cycle. | ||
2. Push the hotfix branch to your personal fork, and open two separate pull | ||
requests: one that merges the hotfix branch into the | ||
`IMAP-Science-Operations-Center` `imap_processing` `main` branch, and one | ||
that merges the hotfix branch into the `IMAP-Science-Operations-Center` | ||
`imap_processing` `dev` branch. | ||
3. For each of these pull requests, proceed with the nominal review & merge | ||
process described in steps (9) and (10) in the previous section. | ||
|
||
|
||
### Keeping your fork updated | ||
|
||
You can keep your personal fork up-to-date with the | ||
`IMAP-Science-Operations-Cetner` `imap_processing` repository by fetching and | ||
bourque marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pulling the `upstream` remote: | ||
|
||
git checkout dev | ||
git fetch upstream dev | ||
git pull upstream/dev | ||
|
||
### Collaborating on someone else's fork | ||
|
||
To contribute to a branch on another person's personal fork, add a new `remote` | ||
that points to their fork, and use the nominal workflow for contributing: | ||
|
||
git remote add <username> <remote url> | ||
git fetch <username> | ||
git checkout -b <branchname> <username>/<branchname> | ||
# Make some changes via add/commit cycle | ||
git push <username> <branchname> | ||
|
||
## Poetry Environment | ||
|
||
TBD | ||
bourque marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## Python Coding | ||
|
||
|
||
`imap_processing` code shall adhere to the [PEP8](https://peps.python.org/pep-0008/) | ||
conventions save for the following exceptions: | ||
|
||
- Lines of code need to be restricted to 88 characters to adhere to the `black` | ||
code formatter. | ||
|
||
Additionally, the code shall adhere to the following special guidelines: | ||
|
||
- Function and class definitions should be placed in alphabetical order in the | ||
module | ||
- It is encouraged to annotate variables and functions using the | ||
[`typing`](https://docs.python.org/3/library/typing.html) library. | ||
|
||
|
||
## API Documentation | ||
|
||
`imap_processing` code shall adhere to the [PEP257](https://peps.python.org/pep-0257/) | ||
and [numpydoc](https://numpydoc.readthedocs.io/en/latest/format.html) conventions. | ||
|
||
The following are further recommendations: | ||
|
||
- Each module should have at minimum a description and a `Use` section. | ||
- Each function/method should have at minimum a description, `Parameters` (if | ||
necessary), and `Returns` (if necessary) sections. | ||
bourque marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## Versioning | ||
|
||
Any changes pushed to the `main` branch should be tagged with a version number. | ||
The version number convention is `x.y.z`, where | ||
|
||
x = The main version number. Increase when making incompatible API changes. | ||
y = The feature number. Increase when change contains a new feature with or without bug fixes. | ||
z = The hotfix number. Increase when change only contains bug fixes. | ||
|
||
|
||
## Security | ||
|
||
The following items should never be committed in the `imap_processing` source | ||
code or GitHub issues/pull requests: | ||
|
||
- Account credentials of any kind (e.g. database usernames/passwords, AWS | ||
credentials, etc.) | ||
- Internal directory structures or filepaths | ||
- Machine names | ||
- Proprietary data | ||
|
||
If `imap_processing` code needs access to this information, it should be stored | ||
in a configuration file that is not part of the repository. | ||
|
||
|
||
## Naming Conventions | ||
|
||
TBD | ||
|
||
## Tools and Library Recommendations | ||
|
||
- `spiceypy` for using SPICE kernels | ||
- `space-packet-parser` to unpack CCSDS packets | ||
|
||
bourque marked this conversation as resolved.
Show resolved
Hide resolved
|
||
## Releases | ||
|
||
TBD |
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.
Would it be appropriate to add a note about testing or documentation here? Or is that not closely related to style? Actually, having a documentation style guide might be a good thing to add... (related to PR #15)
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.
Yeah, now that #15 is merged and we have automated doc builds, I think it makes sense to add a note here about updating the necessary
.rst
files when applicable. I'll add that.Beyond that, and beyond the mention of
PEP257
/numpydocs
style, was there something else we could mention?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.
I think just adding to update or write the documentation as required is all that's needed right now, although I will maybe just write up a documentation specific style guide