Information: Review period of code PRs

[kjetilk]

When we initially established the process, in my mind, a two week review period was for governance changes, which is the main concern of this repository.

For PRs to code, developers expect them to be reviewed and merged promptly, and I reflected that through a commitment from the Inrupt team to the community in this document.

Now, I see that the process could imply that we will not merge any PR towards any Solid repository before two weeks have passed since it was opened. I'm not sure this was the intention, and it is certainly not a workable solution. It would frustrate us who work on the code on a daily basis, as we cannot progress based on previous PRs, it would frustrate occasional contributors, as their code would be left pending for two weeks, and it would prevent us from using any modern software engineering methodology that focuses on short "sprints" and iterations. It is also against current practice.

Now, the most efficient way is to have dialogs open in an early phase. Now, nothing goes into the server without having had an open issue first. Face-to-face meetings are often more efficient to reach agreements, and it is therefore important that decisions can be made in such meetings. There is a possible source of controversy in that.

Other matters of contention does not surface before the code has been written and a PR has been submitted. Team members can and should indicate disagreements by submitting a review that says "changes requested". Others can do so simply by commenting.

But most PRs are not contentious, and should be merged with one or a few "Approved" reviews. I think this should be at the release managers discretion. Things that are obviously contentious would require the Community Leader's approval. I have been quite conscious to make sure this happens.

Now, not everyone have the opportunity to follow the project closely, and comment on an issue or PR in the course of days or hours. I think the key point is to balance the needs of those to the needs of those who submit PRs. In one end of the governance spectrum, a do-o-cratic approach disregards the former group entirely, the other end affords little trust to those who write the code.

I do not think we should be entirely a do-o-cracy, and certainly, when there is contention, a two-week review period is appropriate. I also see it as my responsibility as Release Manager to ensure that people have the opportunity to object, and I think I have a fairly good idea of what will be contentious, though I'm sometimes surprised. But having a two-week review period for any PR would completely stop the project, and so, I think that people would need to be closely involved if they think they should be able to raise an objection to any issue. That is not only because of the progress rate, but also because they need to be familiar with the work to make an informed objection. I think it is an unreasonable expectation for someone who is peripheral to the code base to have as much say on it as someone who follows it on an day-to-day or hour-to-hour basis. Currently, those who do follow the project closely have the opportunity to object. Note that the top bar of Github has links to Issues and Pull Requests. At a minimum, I think people should be expected to use them and the query interface they provide if they want to influence the project. Github also has a notification system, though not ideal, may be used to be notified of changes.

I'm not sure about the exact wording, but I feel strongly that PRs should be reviewed and merged as fast as possible. Usually, the amount of time it takes to merged is constrained by our capacity to do so, and so it scarcely happens too fast. Those who wish to participate must be able to comment in a timely fashion by using the tools that are provided by Github.

[megoth]

I was also of the impression that the process was written with the community-centered repos in mind, that is, PRs closer to organizational changes.

Agreed, code can be seen as implementing rules that change how organization works, but that hard restrictions on how we code is not going to be doable. It will halter the development to a practical stop.

Maybe make the process more clear on this, so it isn't applied on code PRs?

[Mitzi-Laszlo]

As fast as possible? So you would ideally merge instantly after opening a pull request or issue? Do we need a window for dialogue?

[kjetilk]

I'd argue instantly is beyond what is possible. :-) We need to allow for reviewers to make an informed review, but since they already need time to do that, and no merge happens before they have done it, "instantly" is merely hypothetical.

So, I do not entirely agree with @megoth , that we can simply not make the process apply to code, as there are code changes that are contentious.

However, it is also worth noting that the whole point of using a revision system is that code changes are not irreversible. It is not before a code change has made it to a full release that reverting is really that big of a deal, and that takes even longer.

[TallTed]

I think the bottom line here is that there really cannot be a hard-and-fast rule for how long each PR (whether for code or otherwise) should be held before it is merged.

Plenty of code changes, and plenty of human-facing documentation changes, will be non-controversial, and there's no reason to force a 14 day or a 3 day or even a 3 hour delay between PR submission and merge on something like "fixed typo, hyperrlink to hyperlink" ...

PRs should generally be reviewed by someone(s) familiar with the thing(s) being changed -- whether that's code or not. It seems reasonable to say something like "w OR ((x and/or y) AND z) will review all PRs for this repo/file/whatever" -- the timeframe of which reviews might well vary with the reviewer(s) workload of the moment as well as the PR target!

Potentially contentious changes -- of whatever sort -- should be noticed as such by those reviewers, who should be trusted to then request a broader review, potentially including "higher-ups" of whatever sort.

[Mitzi-Laszlo]

The reason why I would opt for a fixed period is that there have been conversations about 1) issues and pull requests not being dealt with fast enough 2) issues and pull requests not being published enough to give a chance to give input and the legitimacy of suggestions being questioned as a result. Not writing it down explicitly means that people working on Solid for a long time have an advantage over new comers who have to guess the unspoken rules of engagement making joining the community not an open transparent environment.

As a solution shall we say that there is a 3 day period for all pull requests and issues to be handled once published?

[kjetilk]

I still have a problem with it as a "one size fits all" solution. I think it will have several detrimental effects to the code quality and the progress of the project.

First of all, there are no unspoken rules of engagement that hasn't been around for at least five years, and it is very standard practice across thousands and thousands of projects. So, granted, if you are completely new to development overall, you will be at a disadvantage, but that is not due to anything particular with Solid, there is always a learning curve involved when entering a new field of endeavor. As I have said, to me it is important that Solid has a very low threshold to entry, but I'm not sure server development is where people should be starting to develop their skills, environments where they would not need to cooperate closely with other developers on the same code base are probably better suited for them.

Many, if not most, PRs will naturally have a review period of more than three days, limited by the availability of reviewers. However, we are also naturally striving for relatively small PRs that are easy to review. It is a good practice to do that, so that you get early feedback and it is not too hard on the reviewers. However, that often means that solving a larger task will involve several, incremental PRs. If each PR requires a three-day review period, it will slow development significantly. If that is the policy, then I suspect that we as developers would end up not writing small PRs, but rather do one large PR so that you would not need to wait for a three-day review period for each.

That would result in larger PRs that are harder to review, a departure from agile development, and a greater risk mistakes will sneak through the review process.

I would be very interested to hear what other larger projects are doing in this regard, but to me, a one-size-fits all approach is neither desirable nor achievable.

It is hard for me to say it, since I am a release manager, but I think @TallTed would support it, the process should to a great extent be at the release manager's discretion. The release manager has to make sure that the best suited reviewers are asked, that a suitable number of reviewers are asked depending on the problem, and that merges doesn't happen before the right number of reviewers have responded with an approval, that merges doesn't happen if there are changes requested, that the right branches are locked to enforce review process, that potentially contentious PRs are brought to e.g. the Community Leader's attention through some means of notification, and indeed, other reviewers are certainly most welcome to do that as well.

[Mitzi-Laszlo]

Just wanted to include the notes from the community support meeting conversation. These points were made by different people within the Solid Team:

• 3 days review period could slow down Solid work.
• What is the practical benefit of a time period? Practical benefit would be to create a more open inclusive environment with transparent decision making process that can be understood and referred back to.
• what is the problem we are trying to solve? Are there other ways to solve it? What we are really trying to solve is that there are constraints when changes are made and they did not have the opportunity to provide their perspective. Time limit is a way to give opportunity. Another approach to invite inclusiveness is to make sure that suggestions are posted in an environment that people can check, for example, solid/suggestions.
  – Discrimination a side effect rather than the root.
• Where is the line between inclusiveness and efficiency? Do-ocracy you need to let people who do stuff decide. Have little autonomy is another extreme we shouldn’t go towards. Metirocracy – do-ocracy. The implementation of these ideals can be toxic to minorities.

• Let’s try to be at the forefront. What can we learn from mistakes. Let’s not do everything like it’s done before, let’s reflect on why we are doing what we are doing. Work on tranlating the specs into natural language. Especially when the specs are evolving, it’s hard to keep them up to date. It could be very lengthly, and easier for people to read, may be too much to chew. Proposing trying to find topics within the spec. Find ways to distribute ways of looking at the spec. Write more tutorials, to solve various problems. There is also room for blog posts explaining elements of the spec. Make it into chewable titbits.

• We can remove commits, they are reversible, however, is reversal practical? Reverting the community commits slow, so will take even longer to reverse. Harder to revert in server because items built on top of previous decisions.

• We are not doing anything differently from another project. Although setting a time period is different from previous practices, what we are trying to do is to build a different model, and previous models have been dominated by a homogenous group, so do we want to use past practices or strive to be on the edge of thought? Could we learn from previous examples how not to fall in the same pitfalls and analyse what did work? We are promising to be different – what worked in the past hasn’t worked.

• Could we differentiate between the repos? If so, how?

• Is the code different from the rules? Node solid server has an impact on society as does the community repo and the Solid spec. So how to differentiate? Difficult to assess what will have a societal impact, especially when professionals in making that assessment cannot access the information because it is not in a language they can interpret.
• Solid server is a normative piece of software that has enormous impact on society, so the code decisions have a large impact.
• The individual pull requests are tiny bits of changes, that does not have that property. They are just there to make the changes incremental to wrap our minds around it. Not the same as a societal change because they are qualitively and quantitively not the same.
• The three spec repos and the community repo should have stricter processes for example with a defined minimum review period and allocated reviewer. The spec has the big normative effect. Good way to distinguish the repos by the normative effect they can have at large. Community repo and solid spec are in that category. So would be good to have a transparent merge.

• Could have a hybrid rule set for node-solid-server saying if the change deviates from the spec, it needs a longer review time. The cases where we implement the specs in the code that deviate from the spec are also important to have a process for.

• Workflow and steps are not documented.

• Engineers are more trusting in a pipeline than non -engineers. Maybe we are trusting on this. Maybe there is a point. Demographics of developer culture, maybe that is because we think it’s just the way it should be.
• Key decisions are disguised as technical when actually they have massive implications beyond technical.
• Code can have tremendous impact on society so important to understand the impact and allow people to chip in.
• Natural language – specs could use some refreshing in any case. In doing that there is a better way to explain thing. Wouldn’t go so far as re-writing technical elements as non-technical. There is opportunity to improve the readability of the spec to ensure that It’s right. Will get a lot of push back saying that the technical will be taken out.
  
  
  
  • We need to build things that are accepted and therefore need to be understandable by all.
  
  

[Mitzi-Laszlo]

@TallTed do you have any thoughts on the meeting minutes in the previous comment?

As a way forward I propose the following:

Pull requests and issues associated to the following repos require to be open for a minimum of three days:
https://github.com/solid/solid-spec
https://github.com/solid/web-access-control-spec
https://github.com/solid/webid-oidc-spec
https://github.com/solid/community

Pull requests and issues associated to all other repos can be closed immediately and do not have a minimum time they have to be left open unless they deviate from the spec in which case they need to be open for a minimum of three days.

Any objections?

[kjetilk]

Works for me. It is also a possibility to add text to the Release Manager's role description, describing what they should do with the remaining repos.

[megoth]

I like this solution as well :smile_cat: Let's write down the reasoning somewhere as well, so that people understand our thinking behind it

[Mitzi-Laszlo]

@kjetilk Yes, I'll add a note to https://github.com/solid/community/pull/44

@megoth ok will include a brief description in the community repo readme

[Mitzi-Laszlo]

https://github.com/solid/community/blob/master/README.md