-
Notifications
You must be signed in to change notification settings - Fork 122
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some details are still unclear to me.
- There is no real explanation for what should be written in the different sections of the decision template
- You say the first version should contain only the problem, but no actual proposed decision. I don't think that keeps the discussion unbiased. It will just be chaos, because people are not biased towards one proposal, but instead are biased towards there own opinions. Often you can't really see whether someone else's idea makes sense without a concrete proposal.
- What is the actual decision process? i.e. who decides whether something gets accepted or not. You said "at least two people" in favor, but is it a simple majority vote? Is there someone who has ultimate decision power (like Linus Torvalds for Linux)?
I still think we should use issues for the initial exploration of a problem. Sometimes the problem is not well defined, other times the problem is clear, but no solution is known. In the first case, a decision PR clearly makes no sense, if we don't have a concrete problem, making a decision is impossible. The second case is especially relevant for things like "we know X is a problem, but we don't have a solution and we don't have the time to find one right now". In those cases, it would just lead to PRs that are open for long periods of time. IMO it is much better to have an issue that says "this is the problem, if you have an idea comment here". Then when someone has a concrete idea, we can start the PR.
Since you mentioned them, for Rust RFCs it is also recommended you discuss them elsewhere (Zulip, Forums, etc.) first, so that you can write a good quality RFC (especially if you are new).
Completely independent of the concrete process, I think we should adopt this rules from the Rust RFCs:
make changes as new commits to the pull request, and leave a comment on the pull request explaining your changes. Specifically, do not squash or rebase commits after they are visible on the pull request.
Also, I think in decision PRs code review comments (i.e. the kind of Github comment that is linked to a line in a file) should be kept to a minimum. If there are concerns that require additional discussion, they should be in "PR root comments" (i.e. not link to code). This reduces the number of different threads, which ultimately are all related since they should discuss a single topic. I tried to follow this rule in this PR. That's why this comment is a bit longer, and the code comments are only concrete questions and suggestions.
doc/decisions/decision_process.md
Outdated
### In Progress | ||
|
||
- You must include all further alternative proposals made in the "Considered Alternatives" section. | ||
- Now it is allowed to have the ??? from the previous round's preferred decision in the decision section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the "???" into the file doesn't make the sentence better ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate the "respond with commit ids" idea. IMO it completely defeats the point of a comment thread. It also means if someone says "this doesn't work because X" you have to fully work out a new solution and update the decision instead of just answering in a comment "do you think Y would be better?". Of course, if you implemented a suggestion (e.g. "why not do Z") exactly, then you don't need to write a full response in a comment. But at least write that you did it as suggested. Just having the commit ids is especially annoying in the notification emails you get from GitHub.
More of a side note, but I think you (partly) misunderstood the reason they suggest discussing Rust RFCs elsewhere first. Yes, low quality RFCs could be rejected. But I think the bigger part (and the part more relevant to us) is that starting with an ill-defined problem and without a clear solution in mind will only lead to needlessly long back and forth in PR reviews. That is why I suggested to use an issue, if the problem isn't clear yet and/or there is no solution proposal.
doc/decisions/decision_process.md
Outdated
- there are several considered alternatives, each with rationale and implication | ||
- decision, rationale and implications is **not** yet filled out if there are people arguing for different options (to keep the discussion unbiased) | ||
|
||
Here "the decision" should not have one decision but several well-described solutions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
several well-described solutions
Where do these come from? Sounds like author has to propose multiple solution. Which would be a waste of time, if everybody already agrees on one solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added to Rationale:
- Several "Related Decisions" are very important even if everyone agrees on one solution.
They allow reviewers and future readers of the decision to understand which options were considered and why they were rejected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite what I meant. The issue is mostly the "well-described" part. It sounds like I have to completely think through at least two solutions, even if it is very obvious that one choice is better than the other.
Maybe something like
Here "the decision" should not have one decision but several well-described solutions. | |
Here "the decision" should not have one decision but should describe multiple possible solutions. | |
The possible solutions have to be described well enough to show why the decision was made. |
This still means we need multiple solutions, but if there is one very obvious best solution the other don't need to be described in detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your fascinating feedback!
I agree to most of it, hopefully clarified the other parts, and think the decision can now pass the first round.
IMO it completely defeats the point of a comment thread.
Unfortunately GitHub has many features that moves information to places only present in GitHub and not in git. Anyway, there is no support from GitHub for what I want, here are 3 workarounds:
diff --git a/doc/decisions/decision_process.md b/doc/decisions/decision_process.md
index 8b4820018..7aae94848 100644
--- a/doc/decisions/decision_process.md
+++ b/doc/decisions/decision_process.md
@@ -13,7 +13,12 @@ But substantial decisions must be made in a transparent and participative way.
- [Documentation guidelines](/doc/contrib/documentation.md) apply.
- During the decision process, the PRs constantly get updated:
- Make changes as new commits to the pull request.
- - Questions in the PRs are answered by updating the PR and answering with the commit id.
+ - Questions in the PRs are answered by:
+ 1. updating the PR
+ 2. either:
+ - accept the suggested change
+ - reply with a quote of the change and the commit SHA-ID
+ - reply with a diff of the change
+ - give a thumbs-up if the change was done exactly as suggested
- As generally recommended in Elektra, do not squash commits after they are visible on the pull request.
- Rebase only if the decision was already accepted and has a merge conflict.
- For reviewers:
@kodebach wrote:
But I think the bigger part (and the part more relevant to us) is that starting with an ill-defined problem and without a clear solution in mind will only lead to needlessly long back and forth in PR reviews
In 8a26880 and 207fb36 I wrote:
- Different to initiatives in Rust, most contributors in Elektra are not experts in configuration management or programming languages.
So we do not expect that a clear problem or solution is in the decision writer's mind beforehand.
Instead the decision process is a supported learning process.
doc/decisions/decision_process.md
Outdated
- there are several considered alternatives, each with rationale and implication | ||
- decision, rationale and implications is **not** yet filled out if there are people arguing for different options (to keep the discussion unbiased) | ||
|
||
Here "the decision" should not have one decision but several well-described solutions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added to Rationale:
- Several "Related Decisions" are very important even if everyone agrees on one solution.
They allow reviewers and future readers of the decision to understand which options were considered and why they were rejected.
I think that over complicates the process a bit. Especially because "either" implies those are the only options. I'd suggest:
I especially don't like the "give a thumbs-up", because it can easily be missed. Marking the thread resolved is better, because it hides the thread by default. If you open a PR, you'll only see what is still unclear/open. Re commit ids & replies: I may have reacted to strongly. Using commit ids as a shortcut is fine, e.g. what you did in your last replies is okay for me. The only thing I really don't like is responding with just a commit id, because that doesn't tell you anything about what happened. For example, "I agree, I added a note" and "I disagree, I added it as a considered alternative" both say much more than just "[commitid]" (with and implied "find out what I did"). |
Co-authored-by: Klemens Böswirth <23529132+kodebach@users.noreply.github.com>
fix small formatting problem
Co-authored-by: Klemens Böswirth <23529132+kodebach@users.noreply.github.com> Co-authored-by: Maximilian Irlinger <maxi6594@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I only made some optional suggestions!
Co-authored-by: Mihael Pranjić <mpranj@limun.org>
Thank you, great work! |
Basics
(added as entry in
doc/news/_preparation_next_release.md
whichcontains
_(my name)_
)Please always add something to the release notes.
(first line should have
module: short statement
syntax)close #X
, are in the commit messages.doc/news/_preparation_next_release.md
scripts/dev/reformat-all
Checklist
(not in the PR description)
Review
Labels