Skip to content
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

Update Coding Guidelines Docs #6

Open
korikuzma opened this issue Jan 23, 2024 · 9 comments
Open

Update Coding Guidelines Docs #6

korikuzma opened this issue Jan 23, 2024 · 9 comments
Assignees

Comments

@korikuzma
Copy link
Contributor

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 korikuzma self-assigned this Jan 23, 2024
@korikuzma
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

+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
Copy link
Member

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

@ahwagner
Copy link
Member

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
Copy link
Member

reece commented Feb 6, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

Copy link

github-actions bot commented May 8, 2024

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.

@github-actions github-actions bot added the stale Issue is stale and subject to automatic closing label May 8, 2024
@jsstevenson jsstevenson removed the stale Issue is stale and subject to automatic closing label May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

4 participants