Conversation
|
|
better-code/book.toml
Outdated
| # Preprocessor to hide `# ` lines in Swift code blocks (like mdbook does for Rust) | ||
| # Only strips lines for HTML output; swift-test backend sees the full code | ||
| [preprocessor.swift-hidden] | ||
| command = "python3 scripts/mdbook-swift-hidden.py" |
There was a problem hiding this comment.
Can you use the built-in mdbook support?
[output.html.code.hidelines]
swift = "#"
There was a problem hiding this comment.
Swift has # directives so might need a different character
There was a problem hiding this comment.
It seems to work! Huge simplification, thanks for pointing that out.
|
I got partway through reading the actual code which began to smell sloppy, and when I reached the point that it decided there was a case it wasn't going to support, and it would just exit with success (the output is not HTML), that was an almost certain sign that this was generated with AI and not carefully reviewed. Please forgive me if I've gotten that wrong, but Everything we check in here, even tooling should uphold the principles of better code. that includes real meaningful contract documentation for example. |
|
Thanks for that feedback @dabrahams - I'll take a closer look and address what you've mentioned. In hindsight I should have made this a draft PR; it wasn't my intention to request a review, just share progress. |
| /// manager of any other employee. | ||
| public remove(_ e: EmployeeID) | ||
|
|
||
| ... |
There was a problem hiding this comment.
I'm not attached to the ellipsis, but do we /have/ to drop it? Something like this tool ought to be able to skip them.
There was a problem hiding this comment.
No, we don't have to drop it. I could add support for simply removing those. Would it be sufficient to remove all lines that contain only whitespace and ... exactly?
| collection type: | ||
|
|
||
| ```swift | ||
| # struct DynamicArray<Element> {} |
There was a problem hiding this comment.
Not PR-related: is it a problem that we're using DynamicArray here and MyArray elsewhere?
|
|
||
| ```swift | ||
| extension Collection { | ||
| extension Collection where Element: Hashable { |
There was a problem hiding this comment.
No. Returning Set<Element> requires the Element to be Hashable.
FAIL: chapter-2-contracts.md:1382
/var/folders/zm/ll4504dd6dgbvy20y9ls2_hr0000gq/T/tmpoeblzo53.swift:2:54: error: type 'Self.Element' does not conform to protocol 'Hashable'
1 | extension Collection {
2 | func bucketed(per bucket: (Element)->Int) -> [Int: Set<Element>] { [:] }
| `- error: type 'Self.Element' does not conform to protocol 'Hashable'
3 | }
4 | (0..<10).bucketed { $0 % 2 }
============================================================
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class TestResult: |
There was a problem hiding this comment.
I don't think this should exist. You should instead propagate exceptions.
There was a problem hiding this comment.
I disagree. Propagating exceptions would make it difficult to collect the test failures and present all of them at once. A propagated-exception model would encourage the caller to bail on the first test failure, but it's useful to users to see all of their failures in one place (so they don't have to needlessly rebuild). To support something like that with an exception propagation model, we'd need a cumbersome try/catch/append at the call site.
scripts/mdbook-swift-test.py
Outdated
| """Yields Swift code blocks from markdown content.""" | ||
| for m in FENCE.finditer(content): | ||
| lang, ignore = parse_attributes(m.group(1)) | ||
| if lang == "swift": |
There was a problem hiding this comment.
If we're only handling swift why is FENCE matching everything and why is parse_attributes returning a language.
There was a problem hiding this comment.
First, creating a regex to match both swift,ignore and ignore,swift (which seemed useful to support) was needlessly complex. Second, it's entirely possible we'll want to add support for other languages (e.g. Dafny, already referenced in this chapter), so the added regex complexity didn't seem worth it.
dabrahams
left a comment
There was a problem hiding this comment.
I feel like I'm still reviewing AI-generated nonsense here. I'm not going further until you tell me I'm wrong, in which case we should probably have teams call to look at the code together and talk it over.
|
Thanks for your feedback @dabrahams. I've resolved or addressed the issues you've expressed so far. |
| // Returns the `i`th element. | ||
| @requires(i >= 0 && i < self.count) | ||
| fun getNth(i: Integer): T | ||
| fun getNth(i: Integer) -> T |
There was a problem hiding this comment.
Wait, @dabrahams is this swift or hylo? I'm seeing fun and : T which makes me unsure.
This PR adds support for checking Swift examples, copying the Rust-mdbook experience as much as possible: lines beginning with
#will be omitted from the book output, but are used to compile the examples. Additionally, as a preprocessor step, all{ ... }blocks are replaced with{ fatalError() }.A block marked with
swift,ignorewill not be built.The examples can be checked with
mdbook build. Several examples were slightly changed to pass the check. Without the two#lines above, the following error is emitted.