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.

Sign up for GitHub

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

Jump to bottom

fixes #312 #314

Merged
merged 4 commits into from
Mar 7, 2016
Merged

fixes #312 #314

merged 4 commits into from
Mar 7, 2016

Conversation

kylekyle
Copy link
Contributor

@kylekyle kylekyle commented Mar 5, 2016

Validate encoding in Content-Type before force_encoding response contents.

@tarcieri
Copy link
Member

tarcieri commented Mar 5, 2016

This is the wrong place to fix this: it's changing the type stored in HTTP::Content#charset from a String to an Encoding. While we could debate the relative merits of what type should've been used, this is already a post-1.0 library and this is a breaking API change (as you can see, it's also causing test failures)

@kylekyle
Copy link
Contributor Author

kylekyle commented Mar 6, 2016

Whoops! Sorry about that. It should always return a String now.

@tarcieri
Copy link
Member

tarcieri commented Mar 7, 2016

Still failing RuboCop due to the inconsistent alignment (also hard tabs). Make sure to run bundle exec rake before committing.

@kylekyle
Copy link
Contributor Author

kylekyle commented Mar 7, 2016

I've never seen RuboCop before. Pretty sweet. It should be passing now.

@tarcieri
Copy link
Member

tarcieri commented Mar 7, 2016

The tests are failing again

@tarcieri
Copy link
Member

tarcieri commented Mar 7, 2016

Also I still think this is the wrong place to solve this: this information should still be available even if the encoding isn't known to Ruby.

Instead I'd suggest handling the force_encoding exception if the encoding isn't known, so it doesn't bubble up.

@kylekyle
Copy link
Contributor Author

kylekyle commented Mar 7, 2016

That's a good point. The charset doesn't change just because Ruby doesn't recognize it as a String encoding. Okay, I moved the fix to right before the force_encoding call.

@tarcieri
Copy link
Member

tarcieri commented Mar 7, 2016

Ok, this looks fine

tarcieri added a commit that referenced this pull request Mar 7, 2016
@tarcieri
Merge pull request #314 from kylekyle/issue-312
438e86a 
fixes #312
@tarcieri tarcieri merged commit 438e86a into httprb:master Mar 7, 2016
@tarcieri
Copy link
Member

tarcieri commented Mar 7, 2016

Thanks!

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Oct 18, 2016
Update ruby-http to 2.0.3.
734018a 
## 2.0.3 (2016-08-03)

* [#365](httprb/http#365)
  Add `HTTP::Response#content_length`
  ([@janko-m])

* [#335](httprb/http#335),
  [#360](httprb/http#360)
  Set `Content-Length: 0` header for `nil` bodies.
  ([@britishtea])


## 2.0.2 (2016-06-24)

* [#353](httprb/http#353)
  Avoid a dependency cycle between Client and Connection classes.
  ([@jhbabon])


## 2.0.1 (2016-05-12)

* [#341](httprb/http#341)
  Refactor some string manipulations so they are more performant
  (up to 3-4x faster) and more concise.
  ([@tonyta])

* [#339](httprb/http#341)
  Always use byte methods when writing/slicing the write buffer.
  ([@zanker])


## 2.0.0 (2016-04-23)

* [#333](httprb/http#333)
  Fix HTTPS request headline when sent via proxy.
  ([@Connorhd])

* [#331](httprb/http#331)
  Add `#informational?`, `#success?`, `#redirect?`, `#client_error?` and
  `#server_error?` helpers to `Response::Status`.
  ([@mwitek])

* [#330](httprb/http#330)
  Support custom CONNECT headers (request/response) during HTTPS proxy requests.
  ([@smudge])

* [#319](httprb/http#319)
  Drop Ruby 1.9.x support.
  ([@ixti])


## 1.0.4 (2016-03-19)

* [#320](httprb/http#320)
  Fix timeout regression.
  ([@tarcieri])


## 1.0.3 (2016-03-16)

* [#314](httprb/http#314)
  Validate charset before forcing encoding.
  ([@kylekyle])

* [#318](httprb/http#318)
  Remove redundant string allocations upon header names normalization.
  ([@ixti])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kylekyle @tarcieri