chore(repo): add PR authoring instructions for charlie#1911
chore(repo): add PR authoring instructions for charlie#1911shellscape merged 1 commit intomasterfrom
Conversation
shellscape
left a comment
shellscape
left a comment
There was a problem hiding this comment.
@CharlieHelps ironically, you did not use the instructions you have just written. analyze the instructions you wrote, and be sure to follow them all.
Expand this to see my work.
|
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, concise guidance overall. The chore(repo) bullets are redundant and can be merged to remove ambiguity. Non-ASCII punctuation in examples and guidance may cause compatibility issues with strict Conventional Commit tooling; prefer ASCII. Minor capitalization/punctuation inconsistencies around “PR template” can be standardized for clarity.
Additional notes (1)
- Style |
.charlie/instructions/pull-requests.md:8-10
Minor consistency issue: capitalize usage of “PR template” consistently and avoid smart quotes/non-ASCII hyphens in this section. This keeps the doc uniform and avoids copy/paste surprises for titles.
Summary of changes
Adds a new repository-scoped guideline document at .charlie/instructions/pull-requests.md describing PR authoring rules. It covers use of the PR template, breaking-change analysis and notation, Conventional Commit title format, when to use chore(repo):, and pre-review local commands (pnpm lint, pnpm fix:js). Includes example PR titles for non-breaking features, breaking changes, and repo maintenance.
| - Use `chore(repo): …` in PR titles for any PR that does not modify package source code (implementation) — docs, CI, release scripts, templates, configs, repo maintenance. | ||
| - If a PR is a repository management task (even when it touches a package’s meta files like `package.json`, `.d.ts`, or config), title it with `chore(repo): …`. |
There was a problem hiding this comment.
The two bullets about chore(repo) (lines 11–12) are redundant and slightly contradictory/ambiguous about what counts as "implementation" versus "meta" (e.g., treating .d.ts as meta). Merging them into a single, explicit rule will reduce confusion and make the guidance easier to follow.
Suggestion
Consider consolidating lines 11–12 into one clearer bullet and avoid em‑dashes by using ASCII-safe wording:
- Use
chore(repo): …for PRs that do not modify a package’s runtime or compile-time implementation. This includes docs, CI, release scripts, templates, configs, metadata (e.g.,package.json,.d.ts), and other repo maintenance.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| - Use lowercase `type` and `scope` (e.g., `fix(node-resolve): …`, `feat(babel): …`). | ||
| - Keep the summary under ~72 characters; be specific and action‑oriented. |
There was a problem hiding this comment.
There are a few non-ASCII punctuation characters that can leak into copied PR titles and trip strict Conventional Commit validators: the ellipses (…) in examples and the non-breaking hyphen in “action‑oriented.” Prefer ASCII punctuation to maximize compatibility and machine-checkability.
Suggestion
Replace non-ASCII punctuation with ASCII equivalents:
- Use lowercase
typeandscope(e.g.,fix(node-resolve): ...,feat(babel): ...). - Keep the summary under ~72 characters; be specific and action-oriented.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| - Always use Conventional Commits format for PR titles: `type(scope): summary`. | ||
| - Use lowercase `type` and `scope` (e.g., `fix(node-resolve): …`, `feat(babel): …`). | ||
| - Keep the summary under ~72 characters; be specific and action‑oriented. |
There was a problem hiding this comment.
This section establishes the title format but lacks enough precision for the "machine‑checkable" goal in the PR description. Explicitly enumerating allowed types for source changes, scoping rules, and title style (imperative mood, no trailing period) will reduce ambiguity and make automated checks reliable.
Suggestion
Consider tightening this section to make it unambiguous and machine‑checkable by adding allowed types, scope rules, and title style, for example:
- Always use Conventional Commits format for PR titles: `type(scope)!: summary` (use `!` only for breaking changes).
- Allowed types for source changes: `feat`, `fix`, `perf`, `refactor`, `revert`. Use `chore(repo)` for all non-implementation changes.
- Scope: the affected package/plugin in dash-case (e.g., `node-resolve`, `babel`). Prefer a single most-impacted scope.
- Summary: imperative mood, no trailing period, ≤72 chars, specific and action-oriented.
- Example machine-check pattern: `^(?:(feat|fix|perf|refactor|revert)\([a-z0-9-]+\)(?:!)?|chore\(repo\)(?:!)?): [^\s].{0,71}$`Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| - Use `chore(repo): …` in PR titles for any PR that does not modify package source code (implementation) — docs, CI, release scripts, templates, configs, repo maintenance. | ||
| - If a PR is a repository management task (even when it touches a package’s meta files like `package.json`, `.d.ts`, or config), title it with `chore(repo): …`. |
There was a problem hiding this comment.
The chore(repo) guidance is clear, but it risks being applied when metadata or .d.ts changes alter public API or runtime behavior, which should not be treated as repo maintenance. Clarifying this exception will prevent misclassification of breaking or user‑visible changes.
Suggestion
Add an explicit exception under this bullet to avoid mislabeling public‑facing changes as chore(repo), for example:
- Use `chore(repo): …` in PR titles for any PR that does not modify package source code (implementation) — docs, CI, release scripts, templates, configs, repo maintenance.
- If a PR is a repository management task (even when it touches a package’s meta files like `package.json`, `.d.ts`, or config), title it with `chore(repo): …`.
- Exception: if a change to metadata or `.d.ts` alters the public API or runtime behavior for consumers, use `feat(<scope>)`/`fix(<scope>)` (and `!` if breaking) instead of `chore(repo)`.Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this clarification.
charliecreates
bot
requested review from
shellscape
and removed request for
CharlieHelps
|
Updated to follow the repo’s PR instructions and template. Thanks for the nudge.
Shout if you want the instructions doc tweaked further (wording, placement, or examples). |
|
Thanks for the call-out, @shellscape. I’ve brought this PR in line with the repo’s own rules and the new instructions.
If you want any tweaks to the new instructions doc (tone, placement, examples), I’m happy to update. |
shellscape
changed the title
2 participants
Rollup Plugin Name:
repoThis PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
resolves #1910
Description
Adds repo-scoped PR authoring instructions under
.charlie/instructions/pull-requests.mdso PR titles, breaking-change handling, and repo-maintenance titles are consistent and easy to verify.Highlights:
type(scope): summary).feat(commonjs)!: …) and include migration notes in the template.chore(repo): …for PRs that don’t modify package source code or are repo management tasks.pnpm lintandpnpm fix:js.Additional notes:
pnpm lint(0 errors, 8 warnings) andpnpm fix:js(no changes needed).