This repository has been archived by the owner on Jun 3, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Discuss rules #67
Discuss rules #67
Changes from 8 commits
ab9456f
7d5ac1c
3b56b4d
8482294
8e7e6c9
b7ba58e
056871b
e63b255
d6d5572
da1d81b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It seems like there's some disagreement about this requirement and others similar to it. I'll try to decompose the sentence according to my literal interpretation:
Emphasis on "2".
Let's call this agent
DeveloperA
."the other" literally refers to another agent different from
DeveloperA
. Let's call thisDeveloperB
.All of the above phrasing to me strongly suggests 2 different agents for this requirement.
Now it might be that someone is in both
@paritytech/locks-review
and@paritytech/polkadot-review
. https://matrix.to/#/!gJeGMHCcDoIwsHIJri:matrix.parity.io/$5H7WC-VGc4Am5HuvivqpuqA-jR_ic2SmWiz0x4HK1cg raised the question:If we'd make it so 1 person can fulfill the criteria by being in both groups, we'd effectively lower the requirement to a single approval (1 person) instead of 2 like I understood by the literal interpretation. If that's intentional, I suggest clarifying the phrasing as e.g.
That removes any implication of "2 team leads" since it only mentions 1 approval from each group, which might be given by the same person if they are in both groups. What's your opinion?
cc @bkchr @TriplEight
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 agree that 1 person that is a member of two teams shall count as a review from both teams. But also it should count as one review of two requested.
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.
Yes that sounds good.
Again to write it in my own words:
@TriplEight @joao-paulo-parity can you than please implement it that way? Ty!
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.
"Either" team? That's different from how I was understanding your proposal.
I think it'll help to walk through an example scenario. Suppose the following structure
Alternative 1 (how it works currently): after
user1
approves we'll still need an approval fromuser2
oruser3
, even thoughuser1
is in both teams.Suggested wording:
Note: I think adding "Each approval should come from a different user" is clearer than saying "2 team leads".
Alternative 2 (what I initially understood from your suggestion):
user1
's approval would fulfill the requirement because they are on both teams, therefore not requiring an approval fromuser2
oruser3
.Suggested wording:
Note: avoids mentioning "2 team leads" in order to not provoke any ambiguity, since a single team lead is enough to fulfill both requirements. Also avoids mentioning "Each approval should come from a different user" since it's intentionally not that way.
Alternative 3 (how I understood from "either"): after
user1
approves we'll need an approval from eitheruser2
oruser3
.Suggested wording:
Which alternative are you suggesting? Also feel free to describe another alternative if none is matching
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.
Isn't alternative 1 and 3 the same?
And is alternative 1 really how it currently works? I doubt this.
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.
Okay, I think I may mixed something up. I'm now confused what the current status is. I just know that yesterday it wasn't alternative 1.
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 have now extended the polkadot-review team and now it should be okay.
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.
In this specific example, when
user1
approves, they result in the same outcome but their definitions are still different.Consider if
user2
were to approve. "Alternative 1" in that case would require that other approval comes from specificallypolkadot-review
(user2
isn't a part of it). However, if you say "total approvals from either team" then the second approval could come from the same team thatuser2
is in. In this reduced example with a few team members they might effectively result in the same conditions, but I think it would be different if you start adding more team members like the groups we have currently.If it's about https://github.com/paritytech/polkadot/runs/5978667098?check_suite_focus=true#step:2:43 it was misbehaving due to a bug, but it should be fine now.
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.
Then let's take alternative 1
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.
FWIW I thought that the initial "🔒️ed lines are reviewed by 2 team leads, one of them must be in @paritytech/locks-review and the other in @paritytech/polkadot-review." was precise enough to imply alternative 1 as-is, but if that is unclear, I'd go for something like:
If that is still unclear, then I'd add somehting like @bkchr said: