[WIP] add contributing guidelines #1580
Conversation
atravitz
force-pushed
the
add/contributing
branch
from
12a30bd to
2503d1c
Compare
mikemhenry
left a comment
mikemhenry
left a comment
There was a problem hiding this comment.
LGTM! Just some comments really
There was a problem hiding this comment.
Do we want a link to this location? I recall github showing a link to this file when someone opens up a PR for the first time.
Or put another way, can you check to see if having a file here does something special? I don't want to maintain two copies which is why a system link might be best
There was a problem hiding this comment.
|
|
||
| ### Installing openfe for development | ||
|
|
||
| ``` bash |
There was a problem hiding this comment.
Opportunity to push for micromamba here
| mamba activate openfe_env # if not already activated | ||
| git clone git@github.com:OpenFreeEnergy/gufe.git | ||
| cd gufe/ | ||
| pip install -e . |
There was a problem hiding this comment.
| pip install -e . | |
| pip install -e --no-deps . |
There was a problem hiding this comment.
This is technically incorrect. It would be pip install -e . --no-deps
There was a problem hiding this comment.
😱 the worst kind of incorrect! I make this mistake all the time and want to guide others on how to troubleshoot the mistake
|
|
||
| ## Contribution Guidelines | ||
|
|
||
| ### Use semantic branch names |
| - CI tests, docs, etc, will run on every PR that has `main` or a branch that begins with `feat/` as a target. | ||
| - Add a news item for any user-facing changes. | ||
| - Rebase or merge, we don't care, but keep your branch up to date with `main`. | ||
| - Always "squash and merge" to keep the git log readable. |
There was a problem hiding this comment.
| - Always "squash and merge" to keep the git log readable. | |
| - Always "squash and merge" when merging pull requests into main or a feature branch to keep the git log readable. This also makes it easier to use the `git bisect` tool. |
|
No API break detected ✅ |
There was a problem hiding this comment.
| ``` | ||
|
|
||
| ### Multi-project development | ||
| It's common to need to develop *openfe** in tandem with another OpenFE Ecosystem package. |
There was a problem hiding this comment.
| It's common to need to develop *openfe** in tandem with another OpenFE Ecosystem package. | |
| It's common to need to develop *openfe** in tandem with another OpenFE Ecosystem package. |
Just a nit, equal empty spaces everywhere.
| ## Contribution Guidelines | ||
|
|
||
| ### Use semantic branch names | ||
| - Start branch names with one of the following "types", followed by a short description or issue number. |
There was a problem hiding this comment.
This is a rather opiniated take on how we should work going forward and I think this needs discussion amongst the team - I, at least in the short term, am not likely to follow this, frankly there's just too much to think about all the time.
| <!-- - PR titles should be essentially a changelog entry. TODO: make this clearer, maybe examples --> | ||
|
|
||
| ### CI, linting, style guide | ||
| - CI tests, docs, etc, will run on every PR that has `main` or a branch that begins with `feat/` as a target. |
There was a problem hiding this comment.
Why just feat? Surely if I want to fix something going into another branch, I would like to run CI on it?
There was a problem hiding this comment.
This is for if it targets feat/*
So like if you make a PR that targets main, CI will run.
If you have a branch called new_feature that targets main in a PR, CI will run.
If you have a branch called new_feature_sub_pr that targets new_feature in a PR, CI will not run.
This is because we have "PRs that target main" listed as the trigger for our CI yaml.
We can add feat/* as a trigger so that if you have a branch called feat/new_feature and then a sub-pr that targets the feat/new_feature branch for a PR, CI will then run.
RE: this guide for branch names, I view this as a guide/reference but not like a "I desk reject PRs that don't follow this convention".
There was a problem hiding this comment.
Same principle applies, there are plenty of fixes where I PR into them. It just seems really odd to isolate one specific sub-type of PR.
There was a problem hiding this comment.
RE: this guide for branch names, I view this as a guide/reference but not like a "I desk reject PRs that don't follow this convention".
The thing is that we can't have two sets of rules for how we work. If we don't engage ourselves to using that system, then we can't tell others that they should be using it.
| - Rebase or merge, we don't care, but keep your branch up to date with `main`. | ||
| - Always "squash and merge" to keep the git log readable. | ||
| - Use [numpy docstrings](https://numpydoc.readthedocs.io/en/latest/format.html) | ||
| - We encourage but do not require [type hints](https://mypy.readthedocs.io/en/stable/cheat_sheet_py3.html). |
There was a problem hiding this comment.
We should require it - anywhere that doesn't currently have type hints either exists in vendored code or is old stuff we didn't get around to adding type hints to.
There was a problem hiding this comment.
+1 to require it (maybe we need to make a few PRs and play with mypy settings but we should start linting/testing for it)
|
|
||
|
|
||
| ### Review process | ||
| - In general, push PRs early so that work is visible, but leave a PR in draft-mode until you want it to be reviewed by the team. Once it's marked as "ready for review," the OpenFE team will assign a reviewer. |
There was a problem hiding this comment.
If the idea is to leave things in draft mode, then I don't think we should be disabling CI for those.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1580 +/- ##
==========================================
- Coverage 95.44% 93.40% -2.04%
==========================================
Files 174 174
Lines 14537 14545 +8
==========================================
- Hits 13875 13586 -289
- Misses 662 959 +297
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
atravitz
changed the title
I'm open to feedback, but I'm starting this PR to formalize much of what I communicated to @ethanholz today.
note that external contributors cannot make sub-issues! #1582 (comment)
Checklist
newsentryDevelopers certificate of origin