Skip to content

Fix incorrect Facts in pkl:base doc comments#1669

Merged
bioball merged 2 commits into
apple:mainfrom
adityasingh2400:fix-stdlib-doc-facts
Jun 11, 2026
Merged

Fix incorrect Facts in pkl:base doc comments#1669
bioball merged 2 commits into
apple:mainfrom
adityasingh2400:fix-stdlib-doc-facts

Conversation

@adityasingh2400

Copy link
Copy Markdown
Contributor

Several Facts: examples in the pkl:base standard library docs assert statements that are false when evaluated. Since these examples are not run by any test, the mistakes went unnoticed and are rendered verbatim into the generated API docs, where they mislead readers.

The corrected examples:

  • String.isNotBlank: "\t\n\r".isNotBlank was listed as holding, but a string of only whitespace is blank, so it is false. Negated it to match the neighboring !"".isNotBlank and !" ".isNotBlank examples.
  • DataSize.toBinaryUnit / toDecimalUnit: the mb/mib lines mirrored the kb/kib lines, but the identity only holds for adjacent units. 1024.kb == 1000.kib (both 1,024,000 b), whereas 1024.mb is 1,024,000,000 b and 1000.mib is 1,048,576,000 b, so they are not equal. There is no clean round-number equivalent at this magnitude, so I removed the two false lines; the remaining examples still demonstrate the conversion.
  • Collection.any: !List(1, 2, 3).any((n) -> n.isEven) is false because 2 is even. Changed the list to List(1, 3, 5) so the negation holds.
  • IntSeq.end: the example read IntSeq(2, 5).start == 5, which documents the wrong property and is false (start is 2). Corrected it to IntSeq(2, 5).end == 5.
  • List.isDistinctBy / distinctBy: List("a", "b", "abc") is not distinct by length, since "a" and "b" both have length 1. Switched to List("a", "bb", "ccc") so the distinctness examples hold.
  • Map: Map(...).values returns a List, not a Set. Corrected the expected type.

I verified that each corrected example evaluates to true, and that the neighboring examples I kept still pass, using the released Pkl 0.31.1 binary. The changes are confined to doc-comment text, so formatting is unaffected.

This reopens #1615, which I had withdrawn, at @bioball's suggestion. The fork backing that PR was deleted in the meantime, so GitHub would not let me reopen it directly. This branch carries the exact same change.

Several Facts: examples in the pkl:base standard library docs assert
statements that are false when evaluated. They are not run by any test,
so the mistakes went unnoticed and render verbatim into the generated
API docs, where they mislead readers. Each corrected example was
verified to evaluate to true with the released Pkl 0.31.1 binary, and
the neighboring kept examples still pass. The changes are confined to
doc-comment text, so formatting is unaffected.

Signed-off-by: Aditya Singh <adisin650@gmail.com>

@bioball bioball left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thank you!

@bioball

bioball commented Jun 9, 2026

Copy link
Copy Markdown
Member

@adityasingh2400 run OVERWRITE_SNIPPETS=1 ./gradlew pkl-core:check to overwrite the snippet tests

Signed-off-by: Aditya Singh <adisin650@gmail.com>
@adityasingh2400

Copy link
Copy Markdown
Contributor Author

Thanks for the review and the approve. I ran OVERWRITE_SNIPPETS=1 ./gradlew pkl-core:check and it regenerated the reflectedDeclaration snippet for the corrected isNotBlank fact. Pushed the updated expectations just now.

@bioball bioball enabled auto-merge (squash) June 11, 2026 17:00
@bioball

bioball commented Jun 11, 2026

Copy link
Copy Markdown
Member

Thanks for fixing these!

@bioball bioball merged commit a9c98e4 into apple:main Jun 11, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants