Fix incorrect Facts in pkl:base doc comments#1615
Closed
adityasingh2400 wants to merge 1 commit into
Closed
Conversation
Several `Facts:` examples in the standard library documentation assert
statements that are actually false when evaluated. Because these examples
are not executed by any test, the errors went unnoticed and are rendered
verbatim into the generated API docs, where they mislead readers.
The corrected examples are:
- `String.isNotBlank`: `"\t\n\r".isNotBlank` is listed as holding, but a
string of only whitespace is blank, so the result is false. Negated to
match the neighboring examples.
- `DataSize.toBinaryUnit`/`toDecimalUnit`: the `mb`/`mib` lines assumed the
same identity as the `kb`/`kib` lines, but `1024.mb` and `1000.mib` are
not equal (the unit-factor ratio is squared at this magnitude). Removed
the two false lines; the remaining examples still demonstrate the method.
- `Collection.any`: `!List(1, 2, 3).any((n) -> n.isEven)` is false because
2 is even. Changed the list so the negation holds.
- `IntSeq.end`: the example read `IntSeq(2, 5).start == 5`, documenting the
wrong property. `start` is 2; `end` is 5.
- `List.isDistinctBy`/`distinctBy`: `List("a", "b", "abc")` is not distinct
by length ("a" and "b" both have length 1). Switched to elements with
distinct lengths so the examples hold.
- `Map`: `values` returns a `List`, not a `Set`.
Verified each corrected example evaluates to true with Pkl 0.31.1.
Contributor
Author
|
Withdrawing this one: these are correct but doc-only Facts corrections in the stdlib, which are better handled as a single maintainer-side cleanup than a one-off external PR. |
Member
|
@adityasingh2400 this is a good PR, why don't you re-open this? |
Contributor
Author
|
Thanks @bioball, happy to. I had withdrawn this one thinking it was better as a maintainer-side cleanup, but if you would like it as a PR I am glad to carry it. The fork that backed this branch was deleted in the meantime, so GitHub would not let me reopen #1615 directly. I have restored the exact same change in #1669. No changes beyond what you reviewed here. |
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.
Several
Facts:examples in thepkl:basestandard 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".isNotBlankwas listed as holding, but a string of only whitespace is blank, so it is false. Negated it to match the neighboring!"".isNotBlankand!" ".isNotBlankexamples.DataSize.toBinaryUnit/toDecimalUnit: themb/miblines mirrored thekb/kiblines, but the identity only holds for adjacent units.1024.kb == 1000.kib(both 1,024,000 b), whereas1024.mbis 1,024,000,000 b and1000.mibis 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 toList(1, 3, 5)so the negation holds.IntSeq.end: the example readIntSeq(2, 5).start == 5, which documents the wrong property and is false (startis 2). Corrected it toIntSeq(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 toList("a", "bb", "ccc")so the distinctness examples hold.Map:Map(...).valuesreturns aList, not aSet. 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.