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

Convenience methods for Obligation and add_observer returning self #86

Merged
merged 3 commits into from
May 20, 2014

Conversation

pitr-ch
Copy link
Member

@pitr-ch pitr-ch commented May 19, 2014

No description provided.

@mighe
Copy link
Contributor

mighe commented May 19, 2014

if Observable#add_observer returns self, we will never be able to remove an observer added as block.

x = o.add_observer { ... }
o.delete_observer(x)

I like very much a fluent interface, but in this case it will make code less flexible

@pitr-ch
Copy link
Member Author

pitr-ch commented May 19, 2014

@mighe Thanks for reviewing. My thinking is: when {} it is probably just an ad hoc observer which does not need removing and reference. Objects as observers can be removed directly. There is also a way around that:

o.add_observer(&(observer = -> { 'a' }))
o.delete_observer observer
# or with Proc auto-detection
o.add_observer(observer = -> { 'a' })
o.delete_observer observer

I would allow o.add_observer(observer = -> { 'a' }) and keep returning self, also because it seems to me that observers are more often created then removed so I'd like to make creation more convenient. What do you think?

@mighe
Copy link
Contributor

mighe commented May 19, 2014

I still don't like the self alternative, the last syntax is somehow clumsy, personally I find it as an "advanced construct" not really easy to read, especially the one with the ampersand.
In my opinion, there's only a little improvement with the self option, but it can lead to memory leaks especially with long living entities like Agent.
For that I still prefer the "old" way, if someone wants to save some keystroke adding observers can use tap.

We're talking about personal taste, so I'd like to hear the opinion from the other team members

@pitr-ch
Copy link
Member Author

pitr-ch commented May 19, 2014

Ok lets wait :) For completeness' sake this was suggested in #73 (comment) to be able to write:

actor.ask :something,
          Ivar.new.add_observer { |time, value, reason| p time, value, reason }

@pitr-ch
Copy link
Member Author

pitr-ch commented May 19, 2014

Just got an idea :)

x.add_observer { 'a' } # => proc
x.with_observer { 'a' } # => x

What do you think? For me this is a winner. Keeps the old behavior and adds what is needed for convenient observer adding.

@jdantonio
Copy link
Member

Good catch, @mighe. When @pitr-ch and I first discussed returning self I failed to consider deleting the observer. Thank you for noting this.

@jdantonio
Copy link
Member

I've opened an Issue for tracking intermittently failing tests and I'm working through those as I find them. Please feel free to add to the list here when you experience these failures.

@pitr-ch
Copy link
Member Author

pitr-ch commented May 20, 2014

@jdantonio thanks, this one is already there so I'll rerun this to get a green here.

@pitr-ch
Copy link
Member Author

pitr-ch commented May 20, 2014

@mighe ack on with_observer?

@mighe
Copy link
Contributor

mighe commented May 20, 2014

Sorry 😊
Yes, it is fine by me, with little documentation it is very clear and unambiguous

@jdantonio
Copy link
Member

👍

wait - waits and returns self
no_error! - waits and raises on rejection, otherwise returns self
value! - returns value, or raises if rejected
exception - allows to call `raise rejected_ivar`
@pitr-ch
Copy link
Member Author

pitr-ch commented May 20, 2014

Thanks, doc added, merging after travis pass.

jdantonio added a commit that referenced this pull request May 20, 2014
Convenience methods for Obligation and add_observer returning self
@jdantonio jdantonio merged commit d1a5954 into ruby-concurrency:master May 20, 2014
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.

None yet

3 participants