biocommons / biocommons.github.io

biocommons website
https://biocommons.org/
Apache License 2.0
1 stars 2 forks source link

Update Coding Guidelines Docs #6

Open korikuzma opened 9 months ago

korikuzma commented 9 months ago

The coding guidelines here should be updated. I would like to create a draft PR for maintainers to review before/during the Feb 12, 2024 devs meeting.

korikuzma commented 9 months ago

Here are some changes I plan to make along with proposing to follow practices that @jsstevenson and I use in our lab repos.

  1. Provide more links / information on why we are making these decisions in the document
  2. Propose a new branch naming convention (shorter), issue-# rather than issue-title-of-issue
  3. Squash + Merge PRs The default merge strategy makes the commit log messy imo. Most of the time, commit messages are not meaningful in PRs. Sometimes, PRs contain several commits for a relatively small change but could really just use a single commit to explain what was done. Our team squashes and merges and uses Conventional Commits. This approach helps provides concise descriptions on what was actually done. It also helps me break up tasks into smaller PRs
  4. Use GitHub to automate release notes
  5. Consider switching to ruff for formatting and linting

@biocommons/maintainers feel free to provide comments here or in the next dev meeting.

korikuzma commented 9 months ago

Squash + Merge PRs The default merge strategy makes the commit log messy imo. Most of the time, commit messages are not meaningful in PRs. Sometimes, PRs contain several commits for a relatively small change but could really just use a single commit to explain what was done. Our team squashes and merges and uses Conventional Commits. This approach helps provides concise descriptions on what was actually done. It also helps me break up tasks into smaller PRs

@ahwagner had asked if we can force squash + merge... The answer is yes!

Screenshot 2024-01-22 at 9 04 24 PM

We can also update what we want the commit to be.

Screenshot 2024-01-22 at 9 06 02 PM

Our lab typically uses squash + merge when merging directly into a branch, (e.g., merging issue-1 into main or issue-2 into dev). If we were merging dev into main, we typically rebase + merge to retain the squashed commits.

jsstevenson commented 9 months ago

+1 to all of the above (especially Ruff!). I'll also add that there is great tooling around conventional commits to support auto-generated releases/versioning/changelogs.

ahwagner commented 9 months ago

This seems like a rational approach towards commit style. And love the advantages this offers for automated release notes... 👍

ahwagner commented 9 months ago

Another thing we should add. Update the PR section of that document with some info about the review process; e.g. how many non-authoring maintainers / codeowners need to review a PR before merge, how PRs should be named (accompanying above recommendations on branch names and conventional commits), etc.

reece commented 8 months ago

I'd like to stick with the github recommended branch names here: image

I'm definitely on board with using GitHub releases.

Also, I have a bunch of notes elsewhere that I'd like to get into the discussion. I'll create a branch and add them for discussion.

image

korikuzma commented 8 months ago

@reece Sounds good. I also have a local branch created. I was expanding on the bullets / providing links. I can make these suggestions if you want to create the draft PR

korikuzma commented 8 months ago

Nice @reece already has an open issue for conventional commits + semantic release here

github-actions[bot] commented 5 months ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.