Skip to content

[Dev-Guide]: Dealing with PR blockers #712

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kirtchev-adacore wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[Dev-Guide]: Dealing with PR blockers #712

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3335b23
[Dev-Guide]: Dealing with PR blockers
kirtchev-adacore Jul 14, 2026
007d2c5
Address feedback
kirtchev-adacore Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/dev-guide.rst
View file
Open in desktop

@tshepang tshepang Jul 15, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we should limit discussion to team meets... I think it's fine to just say "bring up the issue to the team"

View changes since the review

@kirtchev-adacore kirtchev-adacore Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me! Updated.

Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,30 @@ Use the following sentence pattern for multiple syntax categories::

``<Action>`` must denote either ``Changed``, ``New``, or ``Removed``.

Dealing with PR blockers
------------------------

Incorrect or incomplete semantics
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

While working on a PR, a developer may need to modify existing FLS semantics or terminology, only for the developer to discover that these semantics or terms are either incorrect or incomplete.
In such cases, bring up the issue to the FLS team.

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.

@PLeVasseur PLeVasseur Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kirtchev-adacore kirtchev-adacore Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded.


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.

@PLeVasseur PLeVasseur Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.

@kirtchev-adacore kirtchev-adacore Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporated.

This may happen when the Rust Reference or other sources the FLS team may use as reference were changed irrespective of Rust releases.
In such cases, bring up the issue to the FLS team.

Depending on the scope of the required changes, the FLS team may decide to either:
- Enact the necessary changes in the same PR, or
- Create a separate PR to introduce the missing semantics or terms, and then update the contents of the PR-in-development.

Merge ordering
--------------

Expand Down
Loading