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

Nicer error when specifying non-nil non-string opt value #861

Merged
merged 1 commit into from
Oct 4, 2019

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Oct 4, 2019

Previously, if you specified a non-nil non-string opt value, like a
symbol for idempotency_key, you'd get a pretty user-unfriendly error
from Net::HTTP:

/Users/brandur/.rbenv/versions/2.4.5/lib/ruby/2.4.0/net/http/header.rb:21:in `block in initialize_http_header': undefined method `strip' for :foo:Symbol (NoMethodError)

Here, we introduce a new argument error that makes it a little easier
for someone to read. The impetus for the change is that we had an
internal product quality report where someone ran into this and was
confused.

I'm pretty sure this change is backward compatible because Net::HTTP
would call strip on anything that was passed in as a value, and
generally just strings would support that. There may be some other less
common data type that was accidentally compatible that someone was
using, but that case should be quite unusual.

r? @ob-stripe

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

👍

@ob-stripe
Copy link
Contributor

Hm, it looks like the tests are failing in 2.3. Maybe the stdlib didn't strip out nil values in 2.3 and did in later versions? Should we do that ourselves then?

@ob-stripe ob-stripe assigned brandur-stripe and unassigned ob-stripe Oct 4, 2019
Previously, if you specified a non-nil non-string opt value, like a
symbol for `idempotency_key`, you'd get a pretty user-unfriendly error
from `Net::HTTP`:

```
/Users/brandur/.rbenv/versions/2.4.5/lib/ruby/2.4.0/net/http/header.rb:21:in `block in initialize_http_header': undefined method `strip' for :foo:Symbol (NoMethodError)
```

Here, we introduce a new argument error that makes it a little easier
for someone to read. The impetus for the change is that we had an
internal product quality report where someone ran into this and was
confused.

I'm pretty sure this change is backward compatible because `Net::HTTP`
would call `strip` on anything that was passed in as a value, and
generally just strings would support that. There may be some other less
common data type that was accidentally compatible that someone was
using, but that case should be quite unusual.
@brandur-stripe
Copy link
Contributor

Maybe the stdlib didn't strip out nil values in 2.3 and did in later versions? Should we do that ourselves then?

Ah yeah, you're right. I can see the argument for doing it in the API library given that I think that's what we do for nil parameters that are specified.

That said, I think I'm just going to punt on this for now by removing that one line from the test case. In newer versions of Ruby, Net::HTTP's default behavior is pretty reasonable: it strips the nil for you, but then emits a warning, which seems fine.

It someone asks for nil filtering down the line and has a good reason to want it, we can add it in then.

@brandur-stripe
Copy link
Contributor

Thanks!

@brandur-stripe brandur-stripe merged commit bbb585a into master Oct 4, 2019
@brandur-stripe
Copy link
Contributor

Released as 5.6.0.

@ob-stripe ob-stripe deleted the brandur-nicer-error branch January 6, 2020 19:53
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.

4 participants