Refactor triggers to its own class#51
Conversation
Implement status() method assertItemsEqual to avoid order errors
burmanm
force-pushed
the
refactor_alerts
branch
from
195e15e to
cdb0517
Compare
|
overall looks good, I left some comments on the PR, Just one question, if you are testing with 2.0.0.Final-SNAPSHOT, Do you think it worth to update the Travis to test against that version of hawkular? |
|
I can't see any of your comments? As for the Alerts, yes.. I'd say it should be updated (I used the newest docker image). Since that's the target for new versions in any case |
|
|
||
| super(HawkularAlertsClient, self)._setup_path() | ||
|
|
||
| def triggers(self): |
There was a problem hiding this comment.
Why have a method that returns a new instance of AlertsTriggerClient each time it's invoked? This could be just a property with the instance assigned, and it could be used likehawkularclient.triggers.<whatever_triggers_method>instead of hawkularclient.triggers().<whatever_triggers_method> IMHO.
There was a problem hiding this comment.
Yeah, there's no reason to create a new one each time (not that it matters as it does not store any state).
Yeah, I guess triggers.function() would make more sense, I'll change it to that one too
2 participants
Also:
Tested with 2.0.0.Final-SNAPSHOT
@rubenvp8510 Or did you have some better way than .triggers() in mind?