-
Notifications
You must be signed in to change notification settings - Fork 871
Polish up the README #732
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
Polish up the README #732
Conversation
ChristianSi
commented
May 2, 2020
- Break lines that are too long
- Clarify that EOF is allowed after key/value pair
- Fix grammar oddity
- Better explain what the "expected" integer range means
- Add a few hyphens
- Turn internal crossreference into hyperlink
- Avoid duplicate "expressed"
* Break lines that are too long * Clarify that EOF is allowed after key/value pair * Fix grammar oddity * Better explain what the "expected" integer range means * Add a few hyphens * Turn internal crossreference into hyperlink * Avoid duplicate "expressed"
@eksortso: You're misunderstanding about the "should" vs. "must". That was merely a clean-up commit within the PR itself. The actual change is from
(slightly ambiguous and already a source of confusion in the past) to the much clearer
I don't think that's a content change at all, it merely makes the meaning of the non-RFC'd "expected" clearer. (The old wording meant: "all users may expect that a 64 bit range is supported" == "All implementations must fulfill this requirement so as not to violate this expectation".) |
But expectation of a condition makes no claim on what to do when the condition doesn't hold up. I think you're reading a MUST into a weak specification that could be a SHOULD at the very least. |
@eksortso I see that differently, I've always read "expected" in this spec as meaning a requirement rather than a mere recommendation. But I guess that'll be for others to decide. @mojombo might know how that "expected" was originally intended, but I don't know if he's around. Meanwhile you could strengthen your case by finding a TOML implementation that only supports 32-bit integers, silently converting all bigger numbers into ... well, whatever. I sincerely hope you won't find any! But if that's the case than at least we aren't breaking anything by making it clear that this is an requirement. |
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.
LGTM!
README.md
Outdated
TOML implementations must be able to losslessly handle at least all integers in | ||
the 64 bit (signed long) range (−9,223,372,036,854,775,808 to | ||
9,223,372,036,854,775,807). |
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.
Would you be willing to drop this change, for now? We'll tackle this in a follow up, and I'd prefer to not hold up the entire PR over a single point of disagreement. :)
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.
Sure, I've dropped that change for now.
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.
Yay! Thank you! ^>^
I've also pushed a small formatting change: hyphen lines under sections headers are now always as long as the header itself (in a few cases they were longer or shorter). |