Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

Corrections and clarifications of REST API Spec #103

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TallTed
Copy link

@TallTed TallTed commented Jan 22, 2018

Some markup and language tweaks for better reader clarity; some punctuation fixes for typos.

Significant changes to the OPTIONS method section which did not conform to RFC 7231 --
https://tools.ietf.org/html/rfc7231#section-4.3.7 -- as the example was for the specific /data/ container, but the description was server-wide.

Some markup and language tweaks for better reader clarity; some punctuation fixes for typos.

Significant changes to **the `OPTIONS` method** section which did not conform to RFC 7231 --
 https://tools.ietf.org/html/rfc7231#section-4.3.7 -- as the example was for the specific `/data/` container, but the description was server-wide.
@dmitrizagidulin
Copy link
Member

Thanks, @TallTed! Good work.

My only question is - let's make sure that OPTIONS * actually works on node-solid-server; that might not be implemented or supported :)

@TallTed
Copy link
Author

TallTed commented Jan 24, 2018

@dmitrizagidulin - I quite believe that OPTIONS * as in HTTP RFC 7231 section 4.3.7 "Request Methods -- Method Definitions -- OPTIONS" and 7230 section 5.3.4 "Message Routing -- Request Target -- asterisk-form" may not (yet) be implemented or supported in node-solid-server, given that the SoLiD API spec was not in such conformance on this point.

I think such lack should be considered a defect in node-solid-server, and not block correction of the SoLiD API spec, as the HTTP RFCs are prerequisites and thus take precedence over the dependent works.

@dmitrizagidulin
Copy link
Member

Sounds good. If you can, open an issue on node-solid-server to implement OPTIONS *. I agree that the spec should include it.

pfriedman
pfriedman previously approved these changes Oct 18, 2018
Copy link

@pfriedman pfriedman left a comment

Choose a reason for hiding this comment

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

These look like appropriate changes making file read better.

@TallTed
Copy link
Author

TallTed commented Apr 4, 2019

@RubenVerborgh @kjetilk @dmitrizagidulin @elf-pavlik
(in other words, all previous committers)

Conflicts from intervening changes resolved. Whoever can, please merge...

@kjetilk kjetilk added this to the Spec Pull Requests milestone Apr 15, 2019
@TallTed
Copy link
Author

TallTed commented Jul 31, 2019

Again...

@RubenVerborgh @kjetilk @dmitrizagidulin @elf-pavlik
(in other words, all previous committers)

Conflicts from more intervening changes resolved. Whomever can, please merge...

@TallTed
Copy link
Author

TallTed commented Aug 1, 2019

Apparently needs new reviews, since conflict re-resolution...

@pfriedman @dmitrizagidulin @gobengo were positive last round.

Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

Right, so, I don't see anything wrong with it, but also note that these documents needs reworking entirely.

@TallTed
Copy link
Author

TallTed commented Aug 2, 2019

@kjetilk - Understood, about the full rework. I think applying this relatively small adjustment before that full rework will result in a better end than leaving it out until after, at which point the rework may itself be broken because this error persists...

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

Successfully merging this pull request may close these issues.

4 participants