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

Documentation to explain request header casing #1887

Merged
merged 4 commits into from
May 17, 2019

Conversation

carlism
Copy link
Contributor

@carlism carlism commented May 16, 2019

Addresses Issue #1882 Header helper and what it does to case on header names

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Thanks. I made some minor suggestions, appreciate if you could address some of them. Only typos are a must :)


The header name will have been normalized for you.

- In the `header` helper names will be coerced into a capitalized kebab case.
Copy link
Member

Choose a reason for hiding this comment

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

kebab case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's that thing where you use '-' between all words so they look like they're on a kebab. https://medium.com/@pddivine/string-case-styles-camel-pascal-snake-and-kebab-case-981407998841

Copy link
Member

Choose a reason for hiding this comment

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

Oh 🍖

@@ -1822,6 +1822,22 @@ get do
end
```

The above example may have been requested as follows:
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a section called Header Case Handling? This way it appears in the TOC and can be linked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the section heading awkwardly makes it run into the next section on the response headers and I don't want to imply that this handling applies to responses. So I'll add a Request and Response section title as well to be clear. I hope this is ok.

Copy link
Member

Choose a reason for hiding this comment

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

👍

README.md Outdated
- In the `header` helper names will be coerced into a capitalized kebab case.
- In the `env` collection they appear in all uppercase, in snake case, and prefixed with 'HTTP_'.

With this normalization you will be able to handle headers in a case-insensitive way as per
Copy link
Member

Choose a reason for hiding this comment

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

For an API documentation we don't need to convince people this is a good idea. I would put this in the prefix.

The header name will have been normalized per HTTP standards defined in ... regardless of what is being sent by a client.

(fix HTTP type, 2 Ts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updates committed.

CHANGELOG.md Outdated
@@ -5,6 +5,7 @@
* Your contribution here.
* [#1864](https://github.com/ruby-grape/grape/pull/1864): Adds `finally` on the API - [@myxoh](https://github.com/myxoh).
* [#1869](https://github.com/ruby-grape/grape/pull/1869): Fix issue with empty headers after `error!` method call - [@anaumov](https://github.com/anaumov).
* [#1887](https://github.com/ruby-grape/grape/pull/1887): Documentation to explain request header casing - [@carlism](https://github.com/carlism).
Copy link
Member

Choose a reason for hiding this comment

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

No need for a changelog for this. Ignore the bot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thanks

@grape-bot
Copy link

grape-bot commented May 17, 2019

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#1887](https://github.com/ruby-grape/grape/pull/1887): Documentation to explain request header casing - [@carlism](https://github.com/carlism).

Generated by 🚫 danger

@dblock dblock merged commit f021292 into ruby-grape:master May 17, 2019
@dblock
Copy link
Member

dblock commented May 17, 2019

thank you, merged

@carlism carlism deleted the header-documentation branch May 17, 2019 14:28
basjanssen pushed a commit to basjanssen/grape that referenced this pull request Feb 28, 2020
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.

3 participants