Skip to content

Latest commit

 

History

History
157 lines (125 loc) · 8.31 KB

File metadata and controls

157 lines (125 loc) · 8.31 KB

Code review guidelines

Repository layout, commands, and coding conventions are documented in AGENTS.md and the per-package AGENTS.md files under packages/. Read those for context; this file covers only what is specific to reviewing a pull request.

Note that this is a monorepo. Paths below are given in full, because the same filename can exist in more than one package.

General principles

  • Style: Suggest npm run format (from the repo root) or npm run lint-fix for formatting issues; do not comment on individual style nits.
  • Patterns: Prefer established Blockly patterns and the official docs over newly invented conventions. This covers conventions — naming, API shape, file layout — not the quality of surrounding code. Do not ask an author to reproduce a nearby antipattern for the sake of consistency, particularly in tests.
  • Documentation: Prefer linking to the Blockly docs over duplicating content in comments. Flag any new link to developers.google.com/blockly; that site is stale. Existing ones are legacy and can be left alone unless the PR is already touching them.
  • TSDoc: Public APIs require TSDoc for behavior, params, and returns. Do not include implementation details or historical context unless essential.
  • Spelling: colour, neighbour, and centre are spelled the British way throughout the public API (setColour, bumpNeighbours, Align.CENTRE). Every other word is American — initialize, serialize, behavior. CSS properties and values are always American (color, background-color, gray).
  • Dependencies: Flag every new dependency. packages/blockly has no runtime dependencies and must not gain one. A new devDependency anywhere needs its justification in the pull request description.
  • Non-null assertions: Flag new ! assertions and suggest ?. or an explicit guard instead. Existing uses are legacy and are not precedent.
  • Accessibility: Changes that touch the DOM must work for screen reader and keyboard-only users. Look for keyboard reachability, an accessible name, and use of Blockly.utils.aria rather than hand-written aria-* attributes.

Project ownership

RaspberryPiFoundation/blockly is the canonical upstream, maintained by the Raspberry Pi Foundation. It is not a fork of google/blockly. Do not suggest changes on the basis that this is a fork, that upstream should be consulted, or that a change belongs in a Google-maintained repository.

Copyright <year> Google LLC headers on existing files are correct and historical — never ask for them to be changed. Only new files use the Raspberry Pi Foundation copyright. See AGENTS.md.

Pull request descriptions

The description should be concise: what changed, why, and anything a reviewer genuinely needs, such as breaking changes or how to verify non-obvious behavior.

Do not ask authors to expand a description that already covers those things. A short, complete description is correct and preferable. In particular, do not request a rationale narrative, a file-by-file walkthrough, or a restatement of the diff.

Do flag a description that is genuinely missing the why, or one that omits a breaking change.

Generated files

None of these are hand-written, so do not review their contents line by line or ask for edits to them directly. If one looks wrong, the fix belongs in its source.

The question to ask is not "is this file generated?" but "should this file be in this pull request at all, and does it look regenerated rather than hand-edited?"

Expected to appear — review the source, not the file

File Regenerated by
packages/blockly/msg/json/* npm run messages
package-lock.json npm

Do not nitpick their contents. Do flag them if they look hand-edited rather than regenerated — for example msg/json/en.json changing without a corresponding change to packages/blockly/msg/messages.js, an edit to a non-English locale file (those come from TranslateWiki), or a lockfile change in a PR that touches no dependencies. In those cases ask for the file to be regenerated, not patched.

Should not appear at all — flag if present

File Why
**/CHANGELOG.md Written by Lerna at release time, not in feature PRs
packages/docs/docs/reference/** Generated from core TSDoc; gitignored
**/dist/**, **/build/** Build output; gitignored

A change to any of these in a normal pull request means something has gone wrong, and is worth a comment. A hand-written CHANGELOG.md entry in particular should be flagged: release notes come from conventional commit messages, so the fix is to write a good commit message rather than to edit the changelog. The last three are gitignored entirely, so their presence means they were force-added.

The exception is a release pull request, where these files changing is the whole point.

Localization

  • All user-visible strings must use Blockly.Msg, including ARIA labels and other screen-reader-only text.
  • This applies to packages/plugins/* too. Plugins have no message files of their own, so a plugin pull request that adds a string will correctly also change packages/blockly/msg/. Do not flag that as unrelated scope.
  • New strings are hand-written only in packages/blockly/msg/messages.js. packages/blockly/msg/json/en.json and packages/blockly/msg/json/qqq.json are then regenerated by npm run messages.
  • A pull request that adds a string should therefore contain changes to all three files. If en.json or qqq.json is missing, the author likely forgot to run npm run messages; if they were edited by hand, ask for them to be regenerated instead.
  • Link the localization guide if strings are missing or misplaced.
  • PRs that attempt to add translations for non-English strings should be redirected to TranslateWiki, using the same guide.

Plugins (packages/plugins/*)

  • Test files must be named *.mocha.js. The plugin test runner silently finds no tests and exits successfully if they are named anything else, so a green CI run does not prove the tests ran. Flag any new plugin test file that does not use this suffix.
  • New plugins must be TypeScript. Flag a newly added plugin written in JavaScript; the scaffolding generator defaults to JS, so this is an easy one to miss (npx @blockly/create-package plugin <name> --typescript). Existing JavaScript plugins are legacy — do not ask for them to be converted.
  • blockly must be a peer dependency, never a direct dependency. Flag imports that reach into deep paths inside the core package rather than its public entry points.
  • Package names follow the type-based convention in the plugin naming guide. Flag new packages that do not follow it.
  • Hand-edited version fields or CHANGELOG.md entries should be flagged; both are managed by Lerna.

Breaking changes

Policy

  • The definition of a breaking change, and the full list of what does and does not count, is in packages/blockly/AGENTS.md. Use that list rather than a separate one here, so the two cannot drift apart.
  • Prefer deprecation with migration paths over removal.
  • Compatibility: Must support Safari 15.4+, latest Chrome, and latest Firefox.

Identification

Flag breaking changes unless all of the following are true:

  1. The PR description explicitly notes it.
  2. The commit type includes ! (e.g., feat!:).
  3. The target branch is not main.

All three are required. Breaking changes are never merged straight to main: they land on a version branch (for example v14) and are merged to main together when that version is released. So a properly labelled breaking change that still targets main is worth flagging — it is almost always aimed at the wrong branch.