Claude,
For merging to main, we are moving towards "rebase and merge" away from
"Create a merge commit". Squashing and tidy up is probably better done
on the PR before the integration into main.
This is for keeping the long term history for main cleaner.
It may make sense to use a merge commit when history should preserve new
functionality coming in - e.g. something large and significant we'd want
to record.
Choose which you feel is appropriate but "rebase and merge" does produce
more noise in the log history.
Andy
On 23/11/2023 15:22, Bruno Kinoshita wrote:
I think it depends. Sometimes I approve things that look good to me, but
you might still want to request an extra review from Andy or Rob as they
know the code base a lot better.
And this seems to be working.
A review has two aspects
- are there any wider issues? (e.g. it has a new dependency)- "process"
has been followed
- review the code.
If a PR is ready, and only changes a clearly restricted area of code
with no changes outside some subtree or small maven module, and passes
the build - it doesn't bring everything down! - then consider merging it.
In the jargon "RTC", with modest "CTR" if it hangs around too long.
Always RTC would be ideal but we do have to be practical given the
people-resources available.
There are github actions to run the build on a cloned repo or "mavn
clean verify" locally.
Anyone - no just committers - can comment on PRs.
CTR = "Commit Then Review"
RTC = "Review Then Commit"
In the same manner, if you modify the UI and Rob, Andy, or anybody else
reviews it, I am always happy to be added as a second reviewer for
UI/JavaScript if needed.
For the documentation in jena-site, though, pull requests are held back
until we have the code they talk about released, so that the documentation
is not ahead of what users are able to use.
On Thu, 23 Nov 2023 at 13:16, Claude Warren <cla...@xenei.com> wrote:
I haven't been around for awhile so I have a process question.
How many reviews are required before code can be merged?
Claude
--
LinkedIn: http://www.linkedin.com/in/claudewarren
