-
Notifications
You must be signed in to change notification settings - Fork 322
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
Additional breaking changes for 5.x #447
Comments
I think it was somewhat reasonable to not support keyword arguments in 2015. Given it's now 2018, and Ruby 2 was obsoleted for almost two years, I don't think there's a good reason to not support them. It's fine for a HTTP library to be more conservative on Ruby support, but I don't think we should support Ruby versions that Ruby themselves don't support. |
We're already 2.2+ so there aren't any issues with older Ruby versions |
Oh did we bump the min? I missed that sorry! Then we absolutely should move to keyword arguments. |
Totally agree about kwargs! |
Fantastic! |
Yes, I would love to see #377 merged before 4.0, I think it's a good opportunity for that. I will try to help with porting some socket-related fixes from HTTP.rb to Socketry. |
#446 will contain breaking changes. Since we're bumping major versions against at a
x.0.0
release, perhaps it's worth considering if there are any additional breaking changes we want to make before a new major version release.I'll bring up the elephant in the room again: keyword arguments, namely converting the existing option hashes in the API into keyword arguments.
My one sentence argument why: keyword arguments automatically check the validity of all keys passed and raise exceptions automatically for e.g. incorrect argument names, improving correctness. They also simplify passing defaults (no need to use the
opts = { ... }.merge(options)
pattern, and can therefore significantly reduce allocations (going from twoHash
allocations to, depending on the underlying implementation, potentially zero).I am happy to do the work on the conversion, although I'm curious if @ixti has warmed up to them at all over the years as he's voiced opposition in the past.
Another nice thing to integrate with another breaking change cycle is Socketry, particularly since if we're making breaking changes to the timeout API, there might be some additional ones to consider moving to a Socketry-based timeout system.
Thoughts? @ixti @zanker @janko-m
The text was updated successfully, but these errors were encountered: