Skip to content
Merged
Changes from all commits
Commits
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
21 changes: 21 additions & 0 deletions .charlie/instructions/pull-requests.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Pull Requests: house rules

- Always use the repository’s Pull Request (PR) Template. Fill out all sections, including links, context, and checklists. For breaking changes, document the migration clearly in the template.
- Always analyze the change for potential breaking changes. If any public API, runtime behavior, defaults, or supported environments change, treat it as breaking.
- 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.
Comment on lines +6 to +7
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 type and scope (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.

Comment on lines +5 to +7
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

- For breaking changes, use the bang after the scope and annotate the PR template:
- Example: `feat(commonjs)!: drop Node 14 support`
- Also mark the PR Template’s breaking‑change section and include migration notes.
- 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): …`.
Comment on lines +11 to +12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +11 to +12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

- Before marking a PR ready for review, run locally:
- `pnpm lint`
- `pnpm fix:js`

Examples

- Non‑breaking feature to a plugin: `feat(babel): add includeChunks/excludeChunks`
- Breaking change to a plugin: `fix(node-resolve)!: change default resolution for "imports" bare targets`
- Repo maintenance: `chore(repo): update PR template and lint settings`
Loading