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

Refactor common plugin code #40

Merged
merged 9 commits into from
Jun 17, 2016
Merged

Refactor common plugin code #40

merged 9 commits into from
Jun 17, 2016

Conversation

vogdb
Copy link
Collaborator

@vogdb vogdb commented Jun 15, 2016

No description provided.

@vogdb vogdb self-assigned this Jun 15, 2016
@codecov-io
Copy link

codecov-io commented Jun 15, 2016

Current coverage is 92.04%

Merging #40 into master will decrease coverage by 2.21%

@@             master        #40   diff @@
==========================================
  Files            12         12          
  Lines           488        503    +15   
  Methods         124        127     +3   
  Messages          0          0          
  Branches         61         62     +1   
==========================================
+ Hits            460        463     +3   
- Misses           28         40    +12   
  Partials          0          0          

Powered by Codecov. Last updated by 0dedc3b...5c925c3

@vogdb vogdb changed the title Streaming plugin Refactor common plugin code Jun 16, 2016
@vogdb
Copy link
Collaborator Author

vogdb commented Jun 16, 2016

@zazabe please review. It should be easier to review commit-by-commit. Pay special attention to @ 2c11d04

@zazabe
Copy link
Contributor

zazabe commented Jun 17, 2016

  • e3d2c95
    • I wrote a note about having a generic method to send transactions, then i saw your last commit :) (5c925c3)
    • now I'm wondering, is it really useful to have this TCrudPlugin?
    • why not changing sendWithDefaultTransaction(options) into sendWithTransaction(message, callback), then callback could be executed after the common response['plugindata']['data']['error'] check ?
  • 2c11d04:
    OK, get it, I have a better understanding about how it works ;) so if i understand, transaction.execute is just resolving/rejecting the transaction.promise from outside and sendSync has a ref to this transaction.promise and will react to it.
    Sgtm, even if it smells the deferred antipattern a bit, (something external is resolving/rejecting the transaction.promise), it don't think we can really avoid it. We could resolve/reject it based on some event but it will be quite the same :).

@vogdb vogdb mentioned this pull request Jun 17, 2016
@vogdb
Copy link
Collaborator Author

vogdb commented Jun 17, 2016

@zazabe please review

@zazabe
Copy link
Contributor

zazabe commented Jun 17, 2016

lgtm

@vogdb vogdb merged commit abd698f into sjkummer:master Jun 17, 2016
@vogdb vogdb deleted the issue-40 branch June 20, 2016 14:12
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.

3 participants