Release/v4.0.0

[wkdewey]

renaming this branch, also I want to merge the later branches into this one

[techgique]

Will and I met on Zoom to discuss a number of things. These are my notes I prepared for the conversation with a couple edits with information Will shared. We decided it's best to close this and make a new PR with just the newer commits at the end of this branch.

My notes:

• git flow branch naming - "release" branches are only for merging from dev into main. This branch is more focused on facet feature change and a name communicating that would be more helpful
• The first 100-ish commits are old ones that I've already reviewed and are already on dev. There are some commits in here twice. Once towards the very beginning and later there are four commits by Jess that are committed again just before the new ones. Worried these being recommitted again later without others applied in the middle after them could have undesired results.
  • Where did you branch off of for the PR? Should have started off the tail of dev and merged other branches onto that as needed
  • Old branch renamed to release/v4 thus all the old commits
  • I think we want to rebuild this branch and PR by starting a new branch off the tip of dev and cherry-picking the last 20-ish commits that are the part that's actually new
• Your commit pattern makes this hard to review and understand
  • Reviewing commit by commit is hard because each commit is just a small fraction of the overall changes that comprise a feature change / update. Looking at all files changed is a mess because of all the files changed from the first 100-ish commits already on dev
  • Keep interrelated changes for the same functionality together in a single commit. Big changes with lots of lines in the same commit are easier to review than tiny changes spread across 20 commits that are hard to put together into a bigger picture to understand their purpose
  • Some of the changes that are very closely related are a dozen commits apart
  • Git commit messages should help explain purpose and thinking behind decisions of how to approach the code changes
  • Either code should be clear what it's doing from method and variable names, there should be comments alongside the code, or documentation should be committed with or right after code changes in the same branch
    • Example of this critique in other PR: https://github.com/CDRH/orchid/pull/266#pullrequestreview-1186471538
  • Adding PR templates like we use for Open ONI could help on API, Datura, and Orchid repos: https://github.com/open-oni/open-oni/blob/dev/.github/pull_request_template.md
  • byebug lines and things that were undone because they didn't work or were trivial typos or syntax mistakes should be squashed or ignored with git rebase interactive. Not necessary or helpful for other code reviewers / devs to see those aspects of the development process
  • Links to blog posts to help with git commit strategy and making PRs friendly to reviewers
  • Have shared these in the past but I think it was a looong time ago just after you had joined us so it's probably been forgotten
  • https://drewdevault.com/2019/02/25/Using-git-with-discipline.html
  • https://mtlynch.io/code-review-love/
• Engineering of facet handling
  • Code changes in lib/generators/templates/config.rb don't seem to do anything? If they do, it's not explained in the commit or made evident by other code changes. Other facet handling in views builds off of @page_facets which is retrieved from the Orchid::facets and Orchid::facets(section: name) without the wrapping get_facets code
  • Will showed where he made this change in Habeas config.rb. I need to explore this further after reviewing API code.
  • Using array element positions and arrays within arrays is opaque to those reading the code. Rather use a hash with key names that make things clearer from reading the code.
  • Will showed me the config YAML for Habeas and that looks good except we might change "alternate" to something like "aggregation_name" that is more descriptive and would help someone looking at the code connect things between API and Orchid code better
  • Whether this kind of change is possible or helpful TBD
  • Do tests still pass? Haven't checked. We should add or update unit tests for these kinds of changes to the inner workings of API, Datura, and Orchid