Skip to content

Conversation

@absrd
Copy link
Contributor

@absrd absrd commented Mar 3, 2025

No description provided.


if !options.fetch(:raw, false) && res.headers["Content-Type"] =~ CONTENT_TYPE_JSON_REGEX
res.body = JSON.parse(res.body, options[:json_opts] || @connection.data[:json_opts])
if !options.fetch(:raw, false) && res["content-type"] =~ CONTENT_TYPE_JSON_REGEX

Choose a reason for hiding this comment

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

Can we be 100% sure the headers are always lowercase?

Choose a reason for hiding this comment

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

Seems so https://github.com/ruby/net-http/blob/master/lib/net/http/header.rb#L40-L47 or at least they're treated as case-insensitive

end
status_code = res.code.to_i
body = res.body
headers = res.each_header.to_h

Choose a reason for hiding this comment

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

There's a pretty big breaking change here, if we can't guarantee the headers "upper/lower-caseness".

Perhaps we could consider returning a Hash-like object that behaves like Net::HTTPHeader, because otherwise stuff might break when we or externals upgrade to version 4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which is the reason for the major version change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Besides changes in underlying client errors

Choose a reason for hiding this comment

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

shouldn't you be sleeping

@absrd absrd merged commit b8ee978 into master Mar 4, 2025
6 checks passed
@absrd absrd deleted the replace-excon branch March 4, 2025 11:06
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