-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Yield instance to Object#with
block
#50470
Conversation
I'm not really convinced about the usefulness of it, nor it's readability. |
I'd rather have #with return self so you can do |
The idea of |
True. You wouldn't be able to scope it just to the call chain that followed. But I actually do think the original works, even if I don't like the &:ping. Because this is going to be 3.4 code and is quite nice: |
Because let's do the AB: client.with(timeout: 5_000) { client.ping } I prefer the client.with(timeout: 5_000) { it.ping } The longer the class/variable name, the better it gets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I think you convinced me. To be honest I didn't quite forsee the use of with
on an object you'd use in a block, I had more globals in mind.
46af381
to
101b99c
Compare
@@ -28,7 +28,7 @@ def with(**attributes) | |||
old_values[key] = public_send(key) | |||
public_send("#{key}=", value) | |||
end | |||
yield | |||
yield self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the liberty to improve the implementation though, calling yield_safe(&block)
instead of just yield safe
in unnecessary overhead AFAICT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I acknowledge that the change is a mix of optimization (I don't think I'll ever trust my intuition on what is and is not an allocation optimization with regard to blocks) and taste, I wonder if the documentation would benefit from the &block
argument's presence in the method signature. While the examples outline that there's a block involved, I find it easier for readers to grasp when a block argument is explicitly mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's more the role of the documentation. I can update it further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's an improvement!
The introduction of the block argument means that `Object#with` can now accept a `Symbol#to_proc` as the block argument: ```ruby client.with(timeout: 5_000) do |c| c.get("/commits") end ```
101b99c
to
9e64b13
Compare
Detail
The introduction of the block argument means that
Object#with
can now accept aSymbol#to_proc
as the block argument:Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]