[Dev-Guide]: Dealing with PR blockers - #712
Conversation
There was a problem hiding this comment.
not sure we should limit discussion to team meets... I think it's fine to just say "bring up the issue to the team"
There was a problem hiding this comment.
Works for me! Updated.
c697784 to
d90582d
Compare
There was a problem hiding this comment.
approving, but maybe better to wait for @PLeVasseur approval also
d90582d to
3335b23
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
Thanks for taking this up @kirtchev-adacore!
I had some thoughts on where we could tighten things up a bit.
|
|
||
| Depending on the scope of the required changes, the FLS team may decide to either: | ||
| - Enact the necessary changes in the same PR-in-development, or | ||
| - Approximate the contents of the PR-in-development, and open a GitHub issue to track the update on the semantics or terms. |
There was a problem hiding this comment.
Could we state the condition under which deferring the correction is acceptable? Like we chatted about in our meeting, in #694 the leading-:: modeling defect predated the PR and we could keep that PR narrowly scoped without making the defect worse, then track the inherited problem in #711.
As written, "approximate" also appears to permit merging an error introduced by the current PR. I think this option should instead say that a pre-existing, out-of-scope problem may be deferred only when the current PR remains accurate within its stated scope. Otherwise the correction needs to land in the under-review PR or in a prerequisite PR.
I'm a little worried about this being too loose for the conditions under which we can punt to create an issue and address later. Hope you catch my drift.
| Missing semantics | ||
| ~~~~~~~~~~~~~~~~~ | ||
|
|
||
| While working on a PR, a developer may need to modify existing FLS semantics or use existing terminology, only for the developer to discover that these semantics or terms are missing. |
There was a problem hiding this comment.
This was kinda hard to parse, as I couldn't tell how to apply it. Seems this branch details the semantics are described as both existing in the FLS and missing from it, sorta.
Could we state the trigger a little more bluntly / directly? For example:
| While working on a PR, a developer may need to modify existing FLS semantics or use existing terminology, only for the developer to discover that these semantics or terms are missing. | |
| While working on a PR, a developer may discover that the PR depends on semantics or terminology that the FLS does not yet define. |
This would also distinguish this case from incomplete existing text in the preceding subsection.
4 participants
No description provided.