-
Notifications
You must be signed in to change notification settings - Fork 10
Conversation
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.
Found some typos. Otherwise reads well.
docs/migration.md
Outdated
``` | ||
|
||
In previous versions of API Elements, both forms were valid so this is not a | ||
breaking change. However, we found multiple implementation that were fragile |
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.
implementations
docs/migration.md
Outdated
|
||
### Category Element | ||
|
||
The `meta` attribute has been renamed to `metadata` in a category element for |
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.
But minim still calls it meta? Isn't it a discrepancy 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.
I think you maybe confusing meta at the element level, not the attribute called meta in the category element. Which was renamed to prevent confusion between the two.
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.
😄
docs/migration.md
Outdated
### Source Map Element | ||
|
||
A source map element may contain an optional line and column number to make it | ||
easier to handle source map information, computing the line and column number |
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.
- source map information, computing
+ source map information. Computing
docs/migration.md
Outdated
|
||
A source map element may contain an optional line and column number to make it | ||
easier to handle source map information, computing the line and column number | ||
can often be expensive so it may be provider by a parser. Note however that it |
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.
provider -> provided
docs/migration.md
Outdated
|
||
The layout of the enum element has been altered. The enumerations have been | ||
moved to an enumerations attribute of the element and the content represented | ||
the given value, like in other elements. |
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.
the content represented the given value
Reads strangely to me. Perhaps "the content now represents the given value"?
docs/migration.md
Outdated
``` | ||
|
||
The intent of the structure was that it represents an enumeration of north, | ||
east, south and west. As the enumerations do not include a `fixed` type |
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.
Possibly nitpicking, but I thought cardinal points are title cased in English
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.
Since I am refering to the JSON values which are lowercase I don't want to mix the spelling. I've wrapped them in code blocks which I think makes that clearer.
docs/migration.md
Outdated
|
||
The intent of the structure was that it represents an enumeration of north, | ||
east, south and west. As the enumerations do not include a `fixed` type | ||
attribute it represents an enumerations where any string is valid and not just |
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.
enumeration?
Thanks for your review @honzajavorek, I've addressed it all in af40105. |
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 can hardly know whether it's exhaustive (someone from ADT would have to say), but otherwise LGTM!
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.
JSON serialization section can use some rewording. Maybe also add links to the API Elements Spec, like the definition of value
and type
?
docs/migration.md
Outdated
## JSON Serialisation | ||
|
||
In prior versions of API Element, an element could be serialised in JSON as a | ||
JSON type, or an API Element. |
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.
In prior versions of API Elements, an Element (i.e. a type) could be occasionally serialized as a value. This behaviour has since been dropped, so that Elements are always serialized in full form (i.e. as a type, not as a value). For example: ...
What about something like this?
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 that's much better, copied this into da2b009.
This adds a guide which shows all the changes between 0.6 and 1.0. I hope I didn't miss any.