-
Notifications
You must be signed in to change notification settings - Fork 1
Rationale/spec improvements #33
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
Conversation
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
rgommers
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.
Thanks @mgorny. A lot more feedback on this one
| Rationale | ||
| ========= | ||
|
|
||
| Summary of changes |
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.
Was this duplicate, or incomplete? It seems pretty useful to have a concise summary of changes to guide the reader.
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.
It was duplicate to the TOC, and it was pretty arbitrary to begin with. Like, it didn't really focus on what the spec is about, but rather on what you could easily bullet point.
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.
Oh, and the "Overview" section now serves the same purpose. It's a little more verbose, but I think it explains the links better than a list.
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.
Okay, seems reasonable. I'll leave the comment open, since others may have the same question.
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Environment marker usage and optionality is described in "Providers" section. Let's limit "Provider information" only to technical field details themselves. Signed-off-by: Michał Górny <mgorny@quansight.com>
rgommers
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.
LGTM now, thanks @mgorny
Per the feedback, try to make a better split of rationale/specification.
Notably, at this point Rationale tries to introduce all the important concepts, so it's mostly standalone. After reading it, one should have an idea of all the important concepts, and need Specification only to learn the details of how to implement them.
I've also noticed we're not really covering the use of providers in sufficient detail in the specification section, so expanded on the appropriate MUSTs and SHOULDs.
Potential TODO: we may also want to mention environment markers and ABI namespace in Rationale.