docs: update Suggesting Features section with Discussion-first process#9
docs: update Suggesting Features section with Discussion-first process#9lml2468 wants to merge 1 commit into
Conversation
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #9 (.github)
Summary
Documentation-only change that rewrites the Suggesting Features section in CONTRIBUTING.md to describe a Discussion-first workflow, with a link to the (incoming) GOVERNANCE.md in the community repo. The new content is well-structured: 4 numbered steps, an explicit escape hatch for small features, and a friendly "open a Discussion if unsure" closer. Diff is +13/-4, single file, no executable code.
I verified the cross-repo references:
- The
communityrepo and the Ideas discussion category both exist (https://github.com/Mininglamp-OSS/community/discussions/categories/ideasresolves). - The
Feature Requestissue template referenced in the escape-hatch paragraph exists in this repo asISSUE_TEMPLATE/feature_request.yml. - The Octo Board link points to the org-level Projects index, which is fine.
The change is consistent with the existing "Issue or Discussion?" section at the top of the file and with the org's stated direction.
Findings
P1 — GOVERNANCE.md link will 404 if this PR merges before community PR #2
CONTRIBUTING.md line ~81 in the new diff:
> See [GOVERNANCE.md](https://github.com/Mininglamp-OSS/community/blob/main/GOVERNANCE.md) for the full process.The PR body itself acknowledges that GOVERNANCE.md is added by Mininglamp-OSS/community#2, which is still OPEN. Resolving the URL today returns 404. If this PR lands first, the most prominent link in the new section is broken until the community PR merges.
Suggested action: gate this on community#2, e.g. one of:
- Merge ordering — merge
community#2first, this PR second. Simplest. Worth noting in the PR description as a checklist item. - Pin the ref — temporarily point the link at the open PR's branch (
.../community/blob/docs/add-governance/GOVERNANCE.md) and follow up to switch back tomainafter merge. Slightly ugly but rules out a window of brokenness. - Auto-merge with a dependency — set this PR to auto-merge after
community#2, if the org uses that flow.
Option 1 is the cleanest. This is the only finding I'd consider blocking, and it's procedural rather than a defect in the diff itself.
P2 — Minor friction with the existing "Issue or Discussion?" section
Top of the file already says (lines 9–17):
Open an Issue when: … You have a concrete feature request — need is clearly defined with a specific use case
Open a Discussion when: … You have an early-stage idea to share → Ideas
The new section adds a stricter rule:
Significant features require a Discussion before an Issue.
A reader could reasonably hold both rules in their head and still wonder which path applies to a "concrete but significant" feature request. The two sections aren't contradictory — "concrete feature request" can be read as "small, well-scoped feature" — but the boundary is implicit.
Suggested action (non-blocking): either
- Add a one-liner in the new section that bridges to the existing rubric, e.g.: "This refines the rule in Issue or Discussion?: even a concrete feature request that is significant in scope should start as a Discussion.", or
- Tighten the top-of-file bullet to something like: "You have a small, concrete feature request …" so the two sections don't appear to give different answers to the same question.
P2 — "Significant" vs. "minor" is undefined
For minor improvements or small self-contained features, you may open an Issue directly …The line draws a line between "significant" (Discussion required) and "minor / small self-contained" (Issue OK), but doesn't define either. In practice this means contributors have to guess, and maintainers will spend cycles converting Issues to Discussions.
Suggested action (non-blocking): add 2–3 concrete examples or a rough heuristic, e.g. "Roughly: anything that changes a public API, adds a new module, or affects more than one repo is 'significant'. Bug-adjacent improvements, small ergonomic wins, and documentation tweaks are 'minor'." Even one sentence reduces the ambiguity dramatically.
P2 — Step 3 wording is a touch passive
3. **A maintainer converts it.** Once consensus is reached, a maintainer closes the Discussion and creates a tracking Issue …"Closes the Discussion" is a hard call — many projects keep the Discussion open and link the Issue back, so the rationale stays discoverable. If the org has decided to close them, fine; if not, "a maintainer marks the Discussion accepted and opens a tracking Issue linked back to it" is closer to common practice.
This is purely a wording suggestion and depends on what GOVERNANCE.md (in the related PR) actually says — please make sure the two documents agree on this point so contributors don't get conflicting instructions.
Nits (optional)
- The blockquote callout uses
>for two lines; rendering on GitHub will wrap them as a single quoted paragraph. That's fine, but if the intent is two visually distinct lines, add a blank>line between them (> \n>syntax) or put them in separate quoted blocks. - Step 1 ends "…and alternatives you considered." — drop the "you considered" or align with the original text "any alternatives you've considered" for consistency with the bug-report bullet style above.
Verdict
Content of the change is good and aligned with the new governance direction. The only material concern is the GOVERNANCE.md link being live before community#2 merges; resolving the merge order (or pinning the link) closes it. The other items are cleanups that would improve clarity but don't block.
Posting as COMMENTED so the merge-order point is acknowledged before this lands; I'd happily flip to APPROVE once community#2 is queued ahead of this one (or the link is temporarily pinned to the PR branch).
lml2468
force-pushed
the
docs/update-contributing-feature-process
branch
from
200101d to
b7e8c24
Compare
|
Thanks @yujiawei for the review! All items addressed in the amended commit: P1 — GOVERNANCE.md link 404 risk P2 — Bridge with "Issue or Discussion?" (top section) P2 — Define "significant" vs "minor" P2 — Step 3 wording Nit — Step 1 wording |
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #9 (.github)
Summary
Documentation-only change to CONTRIBUTING.md. Replaces the old one-liner under Suggesting Features with a 4-step Discussion-first flow, and tightens the top-of-doc "Open an Issue when" rule for feature requests to "small, concrete" / no cross-repo impact. The intent is clear, the writing is good, and the process described is sensible.
Verdict: Commented — content reads well; one cross-repo sequencing risk and a few minor consistency nits worth addressing before merge.
1. Verification
| Item | Status | Evidence |
|---|---|---|
Diff scope matches PR description (only CONTRIBUTING.md, +15/−5) |
✅ | gh pr diff 9 confirms a single-file change |
| Linked Ideas Discussion category exists | ✅ | Mininglamp-OSS/community Discussion category ideas is present |
Feature Request issue template still referenced and exists |
✅ | Mininglamp-OSS/.github/ISSUE_TEMPLATE/feature_request.yml present |
Linked GOVERNANCE.md exists at the URL given |
❌ | https://github.com/Mininglamp-OSS/community/blob/main/GOVERNANCE.md returns 404 — that file is still in open PR Mininglamp-OSS/community#2 (not merged) |
| Octo Board link points to a real location | ✅ | https://github.com/orgs/Mininglamp-OSS/projects is the org projects page |
| Markdown renders cleanly | ✅ | Blockquote, ordered list, and bold all parse correctly |
2. Findings
P1 — Cross-repo link to a file that does not yet exist on main
CONTRIBUTING.md (post-merge) will link to:
https://github.com/Mininglamp-OSS/community/blob/main/GOVERNANCE.md
That file is currently only on the docs/add-governance branch via Mininglamp-OSS/community#2, which is still open. If this PR (#9) merges before community #2, the link is a hard 404 for the entire window between the two merges. That is the most user-visible failure mode of the new process — a contributor reading the new instructions and clicking the canonical "full process" link will hit a dead page.
Recommendation: make merge order explicit. Either
- Block this PR until
community#2is merged (preferred — single source of truth becomes real before it is advertised), or - Land community#2 first in the same review session and only then merge this PR.
A merge-queue ordering note in the PR description would also be enough.
P2 — Top-of-doc rule does not mention "significant features → Discussion"
The "Issue or Discussion?" guide near the top of CONTRIBUTING.md gets a tighter Issue rule (good), but the parallel "Open a Discussion when:" list is left unchanged. After this PR, the canonical rule for significant features is "open a Discussion", but the only place that says so is several screens down in Suggesting Features. A reader who only consults the top-of-doc decision guide can still miss the rule.
Recommendation: add one bullet to the "Open a Discussion when:" list, e.g.
- You have a **significant feature idea** — changes a public API, adds a new user-facing concept, affects more than one repo, or has notable UX/architectural impact → [Ideas](https://github.com/Mininglamp-OSS/community/discussions/categories/ideas)This keeps the top-of-doc guide self-sufficient and matches the wording later in Suggesting Features.
P2 — Significance criteria worded two different ways
The "Issue or Discussion?" rule defines small as: "well-scoped, no API or cross-repo impact" (CONTRIBUTING.md:11).
Suggesting Features defines significant as: "changes a public API, adds a new user-facing concept, affects more than one repository, or has notable UX or architectural implications" (CONTRIBUTING.md:93).
These should be the inverse of each other but the top version omits the user-facing-concept and UX/architectural axes. Readers comparing the two will assume the difference is intentional ("ah, UX-only changes go through the Issue path because the top guide doesn't list them"), which is the opposite of the intended policy.
Recommendation: unify the criteria — pick the longer list, then either repeat it in both places or have one section refer to the other instead of paraphrasing.
P3 — Mild redundancy in the new section's intro
Lines 81–86 (post-PR) say almost the same thing twice:
> **Significant features require a Discussion before an Issue.**
> This refines the [Issue or Discussion?](#issue-or-discussion) rule above…
Octo follows a **Discussion-first** approach for significant features:The blockquote is already a strong topic sentence; the following paragraph mostly restates it before launching the numbered steps. Consider collapsing to one of the two — either keep the blockquote as the lead-in and start step 1 directly, or drop the blockquote and let the body paragraph carry the rule.
P3 — Process gap: no described path for "rejected" outcomes
Step 3 only describes the accepted branch:
The Project Lead marks the outcome. Once consensus is reached, the Discussion is closed as accepted and a tracking Issue is opened…
What happens to a Discussion that does not reach consensus, is declined, or is parked? Probably this is covered in GOVERNANCE.md, in which case a one-line pointer is enough ("declined / parked outcomes are described in GOVERNANCE.md"). But as written, a contributor whose Discussion goes nowhere has no closing instruction.
P3 — "Project Lead" is referenced but not defined here
The role appears twice in the new copy without definition. Acceptable if GOVERNANCE.md defines it (the link is right there), but the role name is currently load-bearing in this file with no inline gloss. A short parenthetical such as "the Project Lead (see GOVERNANCE.md)" on first use would make the document stand on its own.
3. Suggestions (non-blocking)
- The "Not sure which path to take?" closing line is a nice touch; consider mirroring it at the top of the Issue or Discussion? section as well, so the same advice is reachable from both entry points.
- The "Ideas" category link includes the full URL form; consider using the same shortened anchor style used elsewhere in the file for consistency, or — fine to leave as-is.
4. Additional observations
- No CI / status-check changes are required by this PR; it is doc-only.
- No risk to existing in-flight Issues — the change does not retroactively reroute anything.
- The "Bug Report" section (unchanged) still uses the older imperative-bullet style. Not in this PR's scope, but a future polish pass could harmonize tone with the new Suggesting Features prose.
TL;DR
Solid documentation change. Please address P1 (sequencing the cross-repo link so GOVERNANCE.md exists on main before this lands) and ideally P2 (top-of-doc Discussion rule + unified significance criteria) before merge. The rest is polish.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #9 (.github)
Summary
Documentation-only change to CONTRIBUTING.md. Replaces the old one-liner under Suggesting Features with a 4-step Discussion-first flow, and tightens the top-of-doc "Open an Issue when" rule for feature requests to "small, concrete" / no cross-repo impact. The intent is clear, the writing is good, and the process described is sensible.
Verdict: Commented — content reads well; one cross-repo sequencing risk and a few minor consistency nits worth addressing before merge.
1. Verification
| Item | Status | Evidence |
|---|---|---|
Diff scope matches PR description (only CONTRIBUTING.md, +15/−5) |
✅ | gh pr diff 9 confirms a single-file change |
| Linked Ideas Discussion category exists | ✅ | Mininglamp-OSS/community Discussion category ideas is present |
Feature Request issue template still referenced and exists |
✅ | Mininglamp-OSS/.github/ISSUE_TEMPLATE/feature_request.yml present |
Linked GOVERNANCE.md exists at the URL given |
❌ | https://github.com/Mininglamp-OSS/community/blob/main/GOVERNANCE.md returns 404 — that file is still in open PR Mininglamp-OSS/community#2 (not merged) |
| Octo Board link points to a real location | ✅ | https://github.com/orgs/Mininglamp-OSS/projects is the org projects page |
| Markdown renders cleanly | ✅ | Blockquote, ordered list, and bold all parse correctly |
2. Findings
P1 — Cross-repo link to a file that does not yet exist on main
CONTRIBUTING.md (post-merge) will link to:
https://github.com/Mininglamp-OSS/community/blob/main/GOVERNANCE.md
That file is currently only on the docs/add-governance branch via Mininglamp-OSS/community#2, which is still open. If this PR (#9) merges before community #2, the link is a hard 404 for the entire window between the two merges. That is the most user-visible failure mode of the new process — a contributor reading the new instructions and clicking the canonical "full process" link will hit a dead page.
Recommendation: make merge order explicit. Either
- Block this PR until
community#2is merged (preferred — single source of truth becomes real before it is advertised), or - Land community#2 first in the same review session and only then merge this PR.
A merge-queue ordering note in the PR description would also be enough.
P2 — Top-of-doc rule does not mention "significant features → Discussion"
The "Issue or Discussion?" guide near the top of CONTRIBUTING.md gets a tighter Issue rule (good), but the parallel "Open a Discussion when:" list is left unchanged. After this PR, the canonical rule for significant features is "open a Discussion", but the only place that says so is several screens down in Suggesting Features. A reader who only consults the top-of-doc decision guide can still miss the rule.
Recommendation: add one bullet to the "Open a Discussion when:" list, e.g.
- You have a **significant feature idea** — changes a public API, adds a new user-facing concept, affects more than one repo, or has notable UX/architectural impact → [Ideas](https://github.com/Mininglamp-OSS/community/discussions/categories/ideas)This keeps the top-of-doc guide self-sufficient and matches the wording later in Suggesting Features.
P2 — Significance criteria worded two different ways
The "Issue or Discussion?" rule defines small as: "well-scoped, no API or cross-repo impact" (CONTRIBUTING.md:11).
Suggesting Features defines significant as: "changes a public API, adds a new user-facing concept, affects more than one repository, or has notable UX or architectural implications" (CONTRIBUTING.md:93).
These should be the inverse of each other but the top version omits the user-facing-concept and UX/architectural axes. Readers comparing the two will assume the difference is intentional ("ah, UX-only changes go through the Issue path because the top guide doesn't list them"), which is the opposite of the intended policy.
Recommendation: unify the criteria — pick the longer list, then either repeat it in both places or have one section refer to the other instead of paraphrasing.
P3 — Mild redundancy in the new section's intro
Lines 81–86 (post-PR) say almost the same thing twice:
> **Significant features require a Discussion before an Issue.**
> This refines the [Issue or Discussion?](#issue-or-discussion) rule above…
Octo follows a **Discussion-first** approach for significant features:The blockquote is already a strong topic sentence; the following paragraph mostly restates it before launching the numbered steps. Consider collapsing to one of the two — either keep the blockquote as the lead-in and start step 1 directly, or drop the blockquote and let the body paragraph carry the rule.
P3 — Process gap: no described path for "rejected" outcomes
Step 3 only describes the accepted branch:
The Project Lead marks the outcome. Once consensus is reached, the Discussion is closed as accepted and a tracking Issue is opened…
What happens to a Discussion that does not reach consensus, is declined, or is parked? Probably this is covered in GOVERNANCE.md, in which case a one-line pointer is enough ("declined / parked outcomes are described in GOVERNANCE.md"). But as written, a contributor whose Discussion goes nowhere has no closing instruction.
P3 — "Project Lead" is referenced but not defined here
The role appears twice in the new copy without definition. Acceptable if GOVERNANCE.md defines it (the link is right there), but the role name is currently load-bearing in this file with no inline gloss. A short parenthetical such as "the Project Lead (see GOVERNANCE.md)" on first use would make the document stand on its own.
3. Suggestions (non-blocking)
- The "Not sure which path to take?" closing line is a nice touch; consider mirroring it at the top of the Issue or Discussion? section as well, so the same advice is reachable from both entry points.
- The "Ideas" category link includes the full URL form; consider using the same shortened anchor style used elsewhere in the file for consistency, or — fine to leave as-is.
4. Additional observations
- No CI / status-check changes are required by this PR; it is doc-only.
- No risk to existing in-flight Issues — the change does not retroactively reroute anything.
- The "Bug Report" section (unchanged) still uses the older imperative-bullet style. Not in this PR's scope, but a future polish pass could harmonize tone with the new Suggesting Features prose.
TL;DR
Solid documentation change. Please address P1 (sequencing the cross-repo link so GOVERNANCE.md exists on main before this lands) and ideally P2 (top-of-doc Discussion rule + unified significance criteria) before merge. The rest is polish.
2 participants
Summary
Updates the Suggesting Features section in
CONTRIBUTING.mdto reflect Octo's new Discussion-first governance model.Changes
GOVERNANCE.mdin the community repo for full detailsRelated
GOVERNANCE.md