Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Allow autoRemove from api.subscribe based on callback return values #3752

Merged
merged 12 commits into from
Dec 9, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 8, 2016

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 8, 2016
@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 8, 2016
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 9, 2016
@ngotchac
Copy link
Contributor

ngotchac commented Dec 9, 2016

This would mean that something like this:

contract.Event.subscribe({ toBlock: 'pending' }, (logs) => {
  if (logs.type === 'mined') {
    doStuff(logs);
  }
}, true);

would unsubscribe on first callback call, right?

Don't we want to test if the callback returns a boolean (true IMO) the unsubscribe?
Just seems weird to have to return something if our condition haven't met.

NB : an example with the PR would be nice I guess, so we can really appreciate how it is used.

@ngotchac ngotchac added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 9, 2016
@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Dec 9, 2016
@jacogr
Copy link
Contributor Author

jacogr commented Dec 9, 2016

As discussed - changed check to true and explicitly check for boolean return.

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 9, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 290a4a4 on jg-subscription-check into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 290a4a4 on jg-subscription-check into ** on master**.

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 9, 2016
@gavofyork gavofyork merged commit 45ec84e into master Dec 9, 2016
@gavofyork gavofyork deleted the jg-subscription-check branch December 9, 2016 19:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants