fix: stop inventing a false default for options without one - #2722
Closed
dajiaohuang wants to merge 2 commits into
Closed
dajiaohuang wants to merge 2 commits into
dajiaohuang wants to merge 2 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
`generateConfigOptionsMarkdown()` fell back to the literal string `'false'` when an option declared neither `defaultDescription` nor `default`, so every string, number and array option was documented as having a default of `false`. Of the 45 documented options, 30 declare no default and were affected. The invented value contradicted the option's own description: `--screenshotMaxWidth` is described as "Unset means no resize." on the line directly above "Default: false", and numeric options such as `--screenshotQuality` and `--experimentalScreencastFps` were documented as defaulting to a boolean. Only 6 options genuinely default to `false` (and 8 to `true`, plus `--filesystemRoot`). Emit the line only when a default is actually declared, and regenerate docs/configuration.md. Verified with `npm run gen`, `npm run check-format`, `npm run typecheck` and `git diff --check`. Re-running `npm run gen` is idempotent, so the `[Required] Check docs updated` job stays green.
dajiaohuang
force-pushed
the
fix/generated-config-defaults
branch
from
September 10, 2026 16:15
03ab543 to
7d912c7
Compare
Contributor
|
Hi, can you fix sign the CLA check? That happened because you co-authored commit with noreply@anthropic.com |
dajiaohuang
force-pushed
the
fix/generated-config-defaults
branch
from
September 11, 2026 10:51
feb524b to
df20c95
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
generateConfigOptionsMarkdown()falls back to the literal string'false'when an option declares neitherdefaultDescriptionnordefault:As a result, every string, number and array config option is documented as having a default of
false. Of the 45 documented options, 30 declare no default and are affected. Only 15 declare one (8true, 6false, and--filesystemRootwithdefaultDescription: 'OS temp directory').Relation to #2233 / #2234 — please read before merging
This fallback was introduced deliberately by #2234 (merged 2026-06-19, closing #2233 "Improve configuration section in readme with (implicit) defaults"). #2234 replaced a guarded emission with an unconditional one:
So this PR removes lines that #2234 intentionally added, and I'd rather flag that than present it as an unambiguous bug fix.
The reason I believe it is still a defect: #2233's motivating example was
--memoryDebugging, a boolean whose implicit default genuinely isfalse. Generalising?? 'false'to every option makes the output factually wrong for non-boolean options. For example--screenshotMaxWidth(typenumber) currently reads:and
--screenshotQualityand--experimentalScreencastFpsare numbers documented as defaulting to a boolean.If you would prefer to keep a
Default:line on every option, the alternative is to emit each option's true implicit default (e.g.unset, or wording derived from the option's existing description) rather than omitting the line. I did not attempt that here because it needs per-option judgement — happy to rework the PR that way instead.Verification
npm run gen— completes; the only diff is the 30 removed doc lines plus the generator edit.npm run genis idempotent (sha256ofdocs/configuration.mdunchanged), so[Required] Check docs updatedstays green.npm run check-format— passed.npm run typecheck— passed.git diff --check— passed.No issue is linked; reproduced on the current
main(d05cbc05).