-
Notifications
You must be signed in to change notification settings - Fork 2
Contracts Cleanup #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Made a pass fixing spelling, math expressions (added ketex - see README), made indentation more consistent, removed non-ascii characters in favor of entities, and changed links to markdown format so they can be clicked in the generated docs.
dabrahams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of valuable cleanups (many of which I should have handled myself). Some things to discuss.
| Hoare used this notation, called a “Hoare triple,” | ||
|
|
||
| > {P}S{Q} | ||
| > $\{P\}S\{Q\}$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is objectively harder to read, but as I said I'm OK with changes like this because the math will be formatted better. That said, the use of > was a “I'm doing my best with plain Markdown”-ism. Is there a more appropriate way to set this equation off? I guess “it's a quotation” still applies here…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note also the curlies are missing from the GitHub preview of this file in all the places surrounded by $…$. I don't know if they show up when rendered by mdbook…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent some time experimenting with this to find a solution that both works in mdbook katex and with GitHub Markdown. It isn't pretty. (Push shortly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I tried to suggest elsewhere, maybe we should be postponing any “isn't pretty” changes in favor of whatever makes writing fastest and easiest to understand when reviewing. I agree we should be using LaTeX or some similar syntax for math eventually. I just am not convinced it's a good use of time at this point to spend time changing something N² into some more complex form, especially if it won't easily render everywhere correctly.
| which is an assertion that if **precondition** $P$ is met, operation | ||
| $S$ establishes **postcondition** $Q$. | ||
|
|
||
| <!-- This had been using math for pre and post conditions, but I find mixing math and code makes it look like we are talking about different `x` and $x$ variables and equality vs. assignment gets confusing. I think if the operation is expressed in code, the conditions should be expressed in code. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in principle, and I even tried to do that, but when I looked at the preview the many transitions between gray and white backgrounds wasn't great. I realize that can be fixed with a stylesheet… but also, I think the Markdown processor forced me to add space next to the backticks that made the thing read differently from Hoare's notation. And that certainly is the case with your version. If the argument for math notation is that it's going to be easier to read, I don't think we're winning (yet).
It possibly might be better to get the book written and then take a holistic approach to math. It might be that we want to convert the entire thing to LaTeX, for example. Just a thought. More generally, let's make sure we account for the engineering tradeoffs in our approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sean-parent WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'll have much math, so I'm inclined for now to do what reads well in with mdbook-katex and bend it to also work in githhub markdown. But it is verbose -
Running mdbook is hard to preview - setup instructions in the ReadMe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it's messy and friction-inducing. Why do this now?
| is this *sequencing rule*: | ||
|
|
||
| > {P}S{Q} ∧ {Q}T{R} ⇒ {P}S;T{R} | ||
| > $\{P\}S\{Q\} \wedge \{Q\}T\{R\} \Rightarrow \{P\}S;T\{R\}$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely want good math output for final production, but for the purposes of just reading and reviewing the text and changes, I really don't like the engineering cost I can't easily verify that it means what I originally wrote (and as noted elsewhere GitHub doesn't render the curlies so can't verify that way).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the curlies here too (push pending) - but verbose.
| explicit precondition that isn't implied by the summary: it requires | ||
| that the predicate be a strict weak ordering. | ||
|
|
||
| <!-- SRP: this section bothers me. "among others" instead of fully spelling out the requirements, using (i, j + 1) which may not exist, and the n^2 comparisons without calling out the O(n^3) complexity or which properties could be practically checked. Also is "stable" the term we want to use? Regular and deterministic are also candidates. I've tried to rewrite this a couple of times, but it just gets too complex and the main point is lost. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may have some valid points here but too many complaints and a little to diffuse to deal with here; let's chat in person.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think "deterministic" is a better word than "stable".
| /// - Precondition: `areInIncreasingOrder` is [a strict weak | ||
| /// ordering](https://simple.wikipedia.org/wiki/Strict_weak_ordering) | ||
| /// over the elements of `self`. | ||
| /// - Precondition: `areInIncreasingOrder` is [a strict weak |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for these spaces? I don't see how they're helpful. Personally I think they look quite awkward in the rendered output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches the indentation in the source location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it's consistent with the rest of the chapter (except for the several(!) code blocks where indentation is totally messed up). But maybe we should reconsider and strip the leading indentation from code blocks. Horizontal space is at a premium in print books (do we still believe in those?) and anyway to my eye, isolated indentation looks weird on a gray backgound and does nothing useful on a white background.
|
@sean-parent ping. |
|
@sean-parent I'd like to resolve this PR somehow |
Co-authored-by: Dave Abrahams <dabrahams@adobe.com>
…lab/better-code into sean-parent/contracts-cleanup
| explicit precondition that isn't implied by the summary: it requires | ||
| that the predicate be a strict weak ordering. | ||
|
|
||
| <!-- SRP: this section bothers me. "among others" instead of fully spelling out the requirements, using (i, j + 1) which may not exist, and the n^2 comparisons without calling out the O(n^3) complexity or which properties could be practically checked. Also is "stable" the term we want to use? Regular and deterministic are also candidates. I've tried to rewrite this a couple of times, but it just gets too complex and the main point is lost. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Similar rules apply to the contracts for protocol conformances: a | ||
| Similar rules apply to the contracts for protocol conformance: a | ||
| method satisfying a protocol requirement can have weaker preconditions | ||
| and/or stronger postconditions than required by the protocol: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed we have two colons in this sentence!
| ## Polymorphism and Higher-Order Functions | ||
|
|
||
| Similar rules apply to the contracts for protocol conformances: a | ||
| Similar rules apply to the contracts for protocol conformance: a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intentionally a plural noun. I meant the entities: “protocol conformances” is like “classes”. Does it works as a singular? It no longer refers to entities that way.
Fixed spelling, math expressions (added ketex - see README), made indentation more consistent, removed non-ascii characters in favor of entities, and changed links to markdown format so they can be clicked in the generated docs.
Note
Add math rendering and doc polish
mdbook-katexinstall steps (Windows/macOS/Linux) inREADME.mdandbetter-code/README.md; enable[preprocessor.katex]inbetter-code/book.tomlchapter-2-contracts.md; clarify wording (determinism/transitivity, complexity notes), and fix typos/phrasing.vscode/settings.jsonwith custom dictionary wordsWritten by Cursor Bugbot for commit afd0b3d. This will update automatically on new commits. Configure here.