Code Review Process

Starting a thread to discuss a more formal process to review, test, and accept code that is submitted to the zenon-network repos.

Everyone is welcome to participate.

1 Like

So… now what? :smile:

I wanted to give everyone the opportunity to voice their opinions without my influence, but…

Anyways, after informing us about his plan, Mr Kaine sent invitations to many community developers to help maintain or administrate the official /zenon-network Github repos.

Congratulations to everyone involved!

Now, we need to come to an agreement on code governance.
We cannot expect anyone to override maintainer consensus, even if they have the power to do so.


Each project has an x of n threshold for code acceptance.
To my knowledge, they all require 2 of n maintainers to approve PRs.
The exception is go-zenon, which requires 3 of n approvals.

Consider the Contributing Guidelines and Code of Conduct.

Developers should hold each other accountable for delivering high quality, functional, valuable improvements. Of course, these goals are subjective. As the subject matter expert in your own domain, I know you’ll do your best to ensure the codebase is maintained well.

I think there should be an expressed desire for a particular change before it’s implemented.
This can come from a community member, any developer, anyone; as long as there seems to be a need, we can probably justify the change.
This expression can be made on any social media, though I would prefer if it’s captured in a Github issue to simplify the tracking of progress.
We can also use Github Project boards.

Some updates will take hours to validate and test. We should try to respect each others’ time; be sure that your code is complete and in working condition before asking others to review it.

We may want to adopt a rule that the person who submits a PR cannot also approve it.

When reviewing a PR, try to submit feedback that is more verbose than ‘lgtm’, though it’s probably acceptable in some circumstances, like one-line PRs or text changes.
Use your best judgment and express yourself if you think something should be different.

I’m of the opinion that PRs should be small in size, easy to digest and test, simple to roll back.
There will still be exceptionally large PRs for more complex initiatives, but I ask that we strive to reduce the burden of validation by making incremental code changes.
It’s much easier to verify ten lines of code than 500 or 5000.

I would like us to disclose test plans with our PRs.
This is to keep ourselves accountable for quality assurance, and to inform others that a certain test scope has been covered. We may need multiple participants to validate a PR before it’s accepted into production.


These are all suggestions for how we should proceed.

Try to see this from the perspective of a developer that just discovered Zenon.

  • Who is able to answer their questions?
  • What can they work on?
  • Where can they find more information?
  • How do they contribute?

Some of these questions are out of scope for this topic but we still need to answer them.

2 Likes

If anyone wants insight re: how Bitcoin Core developers have solved this: https://www.youtube.com/watch?v=CJLxwmwL-fw

1 Like

I listened to this. The one take away (for me) is I think we should consider a #zenon-core-dev meeting on matrix. Maybe we can link audio to the meeting too so people can participate by audio and chat.

2 Likes

Here is the next #bitcoin-core-dev meeting (CST). I’m going to attend to see how they go.

1 Like

I consulted Chat GPT and two points stood out.

  1. Avoid Nitpicking: Prioritize major issues over minor style nitpicks, unless coding standards dictate otherwise.

  2. Use Review Tools: Utilize code review tools or platforms to streamline the process and keep track of comments.

Do we have any suggestions for coding standards and tools that can assist with code reviews?

I’m sure we’ll find our way as we start creating, reviewing and merging PRs.

how do you guys feel about a #zenon-dev meeting to discuss development?

1hr max. Voice text options. Host on matrix.

Sure, we can give it a try. I’m not sure what to discuss yet, but voice chat tends to be productive.

2 Likes

We can publish an agenda beforehand. I’m sure we will find things to discuss. I’m going to listen to the BTC meeting this thursday for inspiration.

I agree we need an agenda. There are so many things to do right now.

Example agenda. There is a meeting today starting in 40 minutes. I will try to join.

Here is what a Bitcoin PR review meeting looks like. I cut this off at 5 minutes. Pretty straight forward. There is no audio. Text only. Would you guys like text and audio?

I’d prefer text only. Otherwise we might end up having to do speech-to-text in order to figure out what’s been discussed.

1 Like

here is the output of that meeting yesterday

Bringing this topic back based on our Dev meeting today.

Here is an article that was shared a few months ago on how to contribute Pull Requests to Bitcoin Core.

https://jonatack.github.io/articles/how-to-contribute-pull-requests-to-bitcoin-core

1 Like

Another useful post

1 Like