Skip to content
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

Rename URI to URI-reference, fix JSON Schema #338

Merged
merged 1 commit into from
Nov 8, 2018

Conversation

cneijenhuis
Copy link
Contributor

@cneijenhuis cneijenhuis commented Nov 2, 2018

Following the discussion in #332, we figured out that the spec actually meant to support URI-references all along. However, by defining the type as URI, it is easy to make a mistake and assume it is only a URI, not a URI-reference.
This mistake was also made in the JSON Schema.

This PR therefore does three things:

  • Rename the type to URI-reference, in order to make it easier to grasp for others in the future
  • Fix the JSON Schema
  • Add an example for a relative URI

The main changes are in spec.md and spec.json.

Signed-off-by: Christoph Neijenhuis christoph.neijenhuis@commercetools.de

`Number`) document their `Value` and structured types document their
`Attributes`.
As a convention, extensions of scalar types (e.g. `String`, `Binary`,
`URI-refernce`, `Number`) document their `Value` and structured types document
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing 'e' in "refernce"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

@duglin
Copy link
Collaborator

duglin commented Nov 2, 2018

thanks!!!

Signed-off-by: Christoph Neijenhuis <christoph.neijenhuis@commercetools.de>
@clemensv
Copy link
Contributor

clemensv commented Nov 8, 2018

Yes, it meant to do that. We had a lengthy debate about it these being URI references but that got lost in time. LGTM

@duglin
Copy link
Collaborator

duglin commented Nov 8, 2018

Approved on Nov 8 call

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants