Refactor the spec to better explain the semantic model #497
Conversation
This file contains 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
mtdowling
commented
Jul 14, 2020
| @@ -158,12 +157,6 @@ public Builder selector(Selector selector) { | |||
|
|
|||
| public Builder addConflict(String trait) { | |||
| Objects.requireNonNull(trait); | |||
|
|
|||
| // Use absolute trait names. | |||
| if (!trait.contains("#")) { | |||
There was a problem hiding this comment.
I consider this a bug since it's at odds with how literally every other value is resolved to a shape ID. There were two instances that used this in the prelude that were fixed. I did not see any other cases that would be affected by this in models that I sampled.
johnstonskj
reviewed
Jul 14, 2020
docs/source/1.0/spec/core/model.rst
Outdated
| Shapes | ||
| ====== | ||
| ============ | ||
| Smithy model |
There was a problem hiding this comment.
The title seems very vague, it's not clear which "model" you mean without qualification in the title.
- Change "Smithy overview" to "Smithy framework", which matches the figure caption
- Change "Semantic model" to "The semantic model"
There was a problem hiding this comment.
The title seems very vague, it's not clear which "model" you mean without qualification in the title.
How about "Smithy overview"?
There was a problem hiding this comment.
I kind of like calling this "The Smithy model" still, and making the first sentence something like:
The Smithy model describes the Smithy semantic model and the files used to create it. Smithy models are used to describe services and data structures.
There was a problem hiding this comment.
I dislike the overloading, but I can live with it
johnstonskj
reviewed
Jul 14, 2020
johnstonskj
reviewed
Jul 14, 2020
johnstonskj
reviewed
Jul 14, 2020
mtdowling
force-pushed
the
refactor-spec-for-semantic-model
branch
from
0d4a21a to
4d344f2
Compare
johnstonskj
reviewed
Jul 15, 2020
kstich
requested changes
Jul 15, 2020
4 tasks
kstich
approved these changes
Jul 15, 2020
JordonPhillips
approved these changes
Jul 15, 2020
The semantic model is a much more important concept now that we have two representations for models (the AST and IDL), and these representations are meant to build up the same graph of shapes regardless of their different syntactic features. The spec is also now laid out in an easier to understand way since it explains the high-level concepts, shapes, and traits before diving into the details of the IDL ABNF. Various aspects of the model have been clarified: * Applying traits externally is the same as applying them inline in the semantic model. * Simplifications to the documentation_comment ABNF so that it is now just one of the two kinds of `comment` productions. * A broader overview of the model, including transformations. * More information on namespaces, including that there can be multiple namespaces loaded into the semantic model. * More information on how and why models would be merged together (it's left up to tooling to combine and merge models, but the semantics of merging models are described in the spec). * Lots of minor error corrections in the spec. * The special behavior around resolving relative shape IDs in the "conflicts" property of the `trait` trait have been removed since that is at odds with the rest of the model. This commit moves various headings around and removes some pages, but redirects are issued when a removed page is visited. This commit also ensures that requirements.txt is used for docs. Since we're using a github repo for the redirects package, we need to install it using a special syntax. There some docs around how to pull this off with setup.py, but I couldn't get it to work: https://python-packaging.readthedocs.io/en/latest/dependencies.html#packages-not-on-pypi
mtdowling
force-pushed
the
refactor-spec-for-semantic-model
branch
from
10448b3 to
627e7e9
Compare
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.
The semantic model is a much more important concept now that we have two
representations for models (the AST and IDL), and these representations
are meant to build up the same graph of shapes regardless of their
different syntactic features. The spec is also now laid out in an easier
to understand way since it explains the high-level concepts, shapes, and
traits before diving into the details of the IDL ABNF.
Various aspects of the model have been clarified:
semantic model.
just one of the two kinds of
commentproductions.namespaces loaded into the semantic model.
left up to tooling to combine and merge models, but the semantics of
merging models are described in the spec).
"conflicts" property of the
traittrait have been removed since thatis at odds with the rest of the model.
This commit moves various headings around and removes some pages, but
redirects are issued when a removed page is visited.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.