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

new rule for using content negotiation #686

Merged
merged 12 commits into from
Oct 18, 2021
Merged

Conversation

tfrauenstein
Copy link
Member

No description provided.

@ePaul
Copy link
Member

ePaul commented Oct 5, 2021

👍 in principle.
Maybe also add some links from other parts of the guidelines where it is mentioned.

chapters/data-formats.adoc Outdated Show resolved Hide resolved
chapters/data-formats.adoc Outdated Show resolved Hide resolved
@tfrauenstein
Copy link
Member Author

@ePaul @tkrop updated to reflect your comments -- pls. approve, thx.

Copy link
Member

@tkrop tkrop left a comment

Choose a reason for hiding this comment

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

Can you adapt the text to a line length of 80 characters to make reviewing a bit more convenient?

chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/data-formats.adoc Outdated Show resolved Hide resolved
chapters/data-formats.adoc Outdated Show resolved Hide resolved
@tkrop
Copy link
Member

tkrop commented Oct 7, 2021

👍

chapters/common-headers.adoc Outdated Show resolved Hide resolved
Comment on lines -84 to +85
requested URI can be used to indicate that the returned resource is subject
to content negotiations, and that the value provides a more specific
requested URL can be used to indicate that the returned resource is subject
to <<244, content negotiations>>, and that the value provides a more specific
Copy link
Member

Choose a reason for hiding this comment

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

Why change URI to URL? (I guess in this case it's both, but I think URI is more commonly used nowadays (and also in the next bullet point.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that URI is more used than URL. In our Guidelines I see 20 URI and 22 URL occurrences.
I try to consistently prefer the more specific URL term, if appropriate.
Therefore changed here.

Copy link
Member

Choose a reason for hiding this comment

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

I would use URI and URL in there different meanings as appropriate. The point here is, that we talk about URL in case of endpoints since they are meant to be resolved via the HTTP protocol. In cases where we just identify the resource without telling whether we plan to make it accessible via HTTP, I would use URI.

Comment on lines -22 to +33
== {MUST} property names must be ASCII snake_case (and never camelCase): `^[a-z_][a-z_0-9]*$`
== {MUST} property names must be snake_case (and never camelCase)

Property names are restricted to ASCII strings. The first
character must be a letter, or an underscore, and subsequent
Property names are restricted to ASCII snake_case strings matching regex `^[a-z_][a-z_0-9]*$`.
The first character must be a lower case letter, or an underscore, and subsequent
characters can be a letter, an underscore, or a number.

(It is recommended to use `_` at the start of property names only for keywords like `_links`.)
Examples:

[source]
----
customer_number, sales_order_number, billing_address
----
Copy link
Member

Choose a reason for hiding this comment

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

the change in this file seems unrelated to the rest of the changes, and I'm also not sure it's an improvement.

We had this regexp intentionally as part of the header, so it's visible already in the table of contents, and now you've moved it into the text?

Copy link
Member Author

Choose a reason for hiding this comment

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

a) Yes, this change is not related to rest of the changes -- my fault, I originally intended to do the commit on the branch of parallel #690
b) I moved the regex from the header into the text because (i) it IMO improves readability -- being a regex expert is not required to apply the guidelines, and (ii) to be consistent with other rules like
https://opensource.zalando.com/restful-api-guidelines/#240
https://opensource.zalando.com/restful-api-guidelines/#130
https://opensource.zalando.com/restful-api-guidelines/#215
https://opensource.zalando.com/restful-api-guidelines/#129

@ePaul
Copy link
Member

ePaul commented Oct 12, 2021

👍


Property names are restricted to ASCII strings. The first
character must be a letter, or an underscore, and subsequent
Property names are restricted to ASCII snake_case strings matching regex `^[a-z_][a-z_0-9]*$`.
Copy link
Member

Choose a reason for hiding this comment

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

@ePaul I'm wondering whether you can remove the leading _ when we get completely get rid of HAL after your have created the pull request for #483.

@tkrop
Copy link
Member

tkrop commented Oct 18, 2021

👍

@tfrauenstein tfrauenstein merged commit 78960c2 into master Oct 18, 2021
@tfrauenstein tfrauenstein deleted the tfrauenstein-patch-4 branch November 1, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants