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

V1 specification #151

Merged
merged 19 commits into from May 2, 2024
Merged

V1 specification #151

merged 19 commits into from May 2, 2024

Conversation

yamahito
Copy link
Collaborator

@yamahito yamahito commented Apr 9, 2024

Updating Respec and fixing errors in V1 of the spec

@yamahito yamahito requested a review from adamretter April 9, 2024 08:58
Copy link
Member

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

Thanks. See feedback comments for some corrections that are needed please.

@adamretter
Copy link
Member

@yamahito Can you update your commit messages to the standard form we use please (see: https://github.com/eXist-db/exist/blob/develop/CONTRIBUTING.md#commit-labels)

@yamahito
Copy link
Collaborator Author

@yamahito Can you update your commit messages to the standard form we use please (see: https://github.com/eXist-db/exist/blob/develop/CONTRIBUTING.md#commit-labels)

@adamretter I have rebased to reword the commit messages, but it does still leave the old commits in the PR history: is this OK, or do you know how to remove the older commits with the incorrect wording?

@adamretter
Copy link
Member

adamretter commented Apr 30, 2024

I have rebased to reword the commit messages, but it does still leave the old commits in the PR history

@yamahito Erm I am not really sure how that is possible... Can you describe what you did exactly and maybe I can help figure out what went wrong here?

p.s. Your commit messages seems to still have a : after the label, and they should consistently start with a capital letter after the label please.

@yamahito
Copy link
Collaborator Author

I have rebased to reword the commit messages, but it does still leave the old commits in the PR history

@yamahito Erm I am not really sure how that is possible... Can you describe what you did exactly and maybe I can help figure out what went wrong here?

p.s. Your commit messages seems to still have a : after the label, and they should consistently start with a capital letter after the label please.

Never mind, fixed it! I will go and amend those colons and capitals now I know how to do what I need.

@yamahito
Copy link
Collaborator Author

@adamretter I've been able to rebase my local git and force push the reworded commit messages to this branch: I believe that all the changes with the correct commit history (and messages) are now committed. I will add those changes from K-2963 to this PR that you've identified elsewhere and re-request the review.

@yamahito yamahito dismissed adamretter’s stale review April 30, 2024 15:11

Going to add changes from K-2963

@yamahito yamahito marked this pull request as draft April 30, 2024 15:11
- this matches the original RESTXQ paper
- Adds content negotiation section to better reflect the original paper, and adds references between new sections and the annotations.
- Adds (replicates) example from %rest:produces to %rest:consumes and new content negotiation section
@yamahito yamahito marked this pull request as ready for review April 30, 2024 17:13
@yamahito
Copy link
Collaborator Author

@adamretter I have now added the actions from K-2963 and this is again ready for review.

There was some refactoring of the %rest:consumes and %rest:produces annotations to group them with other annotation definitions, which is why I think the content negotiation preamble was missing. I have elected to add this as a new section, which could be expanded upon in the 2.0 spec. Let me know if you feel this is the wrong approach.

I also spotted what I believe are some spelling errors, again, please let me know if this is specialised vocabulary that I have mistakenly "corrected".

Copy link
Member

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

Good improvements, but some changes still needed please.

fn:collection("/db/widgets")/widgets
};
</pre>
<p>The mechanism by which formats are converted are implementation dependent.</p>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that is entirely correct, as it is also influenced by the %output:method annotation. I would suggest removing this paragraph please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK; I wanted to clarify that the (automatic) representation of e.g. an XML fragment as a JSON map (such as is supported natively by eXist-db) was outside the scope of the specification. For now I will remove it as you suggest.

@@ -903,6 +903,13 @@ <h4>rest:uri()</h4>
this is the <code>rest:base-uri()</code> appended with the path from the Path Annotation (if present) of the Resource Function.
</p>
</section>
<section>
<h4>rest:build-uri()</h4>
Copy link
Member

Choose a reason for hiding this comment

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

This does not match the example below!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops. Will fix.

<section>
<h4>rest:build-uri()</h4>
<div class="exampleInner">
<pre><code class="function">rest:get-absolute-uri($path-segments as <code class="type">xs:anyAtomicType+</code>)</code> as <code class="type">xs:anyURI</code></pre>
Copy link
Member

Choose a reason for hiding this comment

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

In K-2963 we suggested it should instead be named rest:build-absolute-uri, please fix this.

@yamahito yamahito requested a review from adamretter May 2, 2024 10:32
Copy link
Member

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @yamahito

@adamretter adamretter merged commit 675529e into exquery:main May 2, 2024
3 checks passed
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

2 participants