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

Update spec.md #333

Closed
wants to merge 3 commits into from
Closed

Update spec.md #333

wants to merge 3 commits into from

Conversation

fabiojose
Copy link
Contributor

Some improvements in the event serialized as JSON.

The proposed updates are based in the issue #332.

Some improvements in the event serialized as JSON.
Signed-off-by: Fabio José <fabiojose@gmail.com>
@duglin
Copy link
Collaborator

duglin commented Oct 24, 2018

@fabiojose please sign your commits

spec.md Outdated

## Using relative URI in `source` element

As defined in [RFC 3986, Section 4.2](https://tools.ietf.org/html/rfc3986#section-4.2) it is possible to use relative paths to express URI, but this require the apply of resolution algorithm [RFC 3986, Section 5](https://tools.ietf.org/html/rfc3986#section-5).
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you please wrap this at 80 columns to align with our other docs?

spec.md Outdated
@@ -290,3 +294,43 @@ The following example shows a CloudEvent serialized as JSON:
"data" : "<much wow=\"xml\"/>"
}
```

Regarding the resolution algorithm, lets consider:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This section confuses me. It sounds a bit like text that should be in the main part of the spec, not in an example section, because its trying to explain how to resolve relative URIs - which feels like normative language. But, it's also just reiterating what RFC3986 says - so why do we need to repeat it?

Also, where is "base URI" coming from? We don't have that defined as part of the spec.

Perhaps it would help me if you could explain what you're trying to fix, or clarify, in the spec with this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR is based on the issue 332 opened by. The example of serialized JSON is not valid against the JSON Schema, because of source element value, which is /mycontext.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup - but I think an easier solution is to just change the example to use a real URI or to make the type of "source" into a URI-reference. Getting into URI resolution seems unnecessary to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree with you @duglin!

I will refactor the PR!

Signed-off-by: Fabio José <fabiojose@gmail.com>
@fabiojose
Copy link
Contributor Author

Closing this to open another one with properly changes.

@fabiojose fabiojose closed this Oct 25, 2018
@fabiojose fabiojose deleted the patch-1 branch October 25, 2018 01:12
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.

2 participants