Implement ConditionRefresh2 #1353
Conversation
|
This pull request introduces 2 alerts when merging a870980 into cb1abb1 - view on LGTM.com new alerts:
|
|
This pull request fixes 1 alert when merging a71e649 into cb1abb1 - view on LGTM.com fixed alerts:
|
|
Hi @Archie-Miller , I guess there is some CTT tests which cover test cases for this functionality? |
|
Hello Martin, There are ConditionRefresh and ConditionRefresh2 tests, however I only ran a subset. This pull request should wait until I have a chance to run all the existing tests. I’m out of the office and should be able to run the tests during our next call on Thursday. Archie |
|
This pull request fixes 1 alert when merging 8720ee4 into 558c395 - view on LGTM.com fixed alerts:
|
|
Hello @mregen I have confirmed the operation of ConditionRefresh and ConditionRefresh2. I have also refactored the section that I believe you found most problematic. |
| /// <summary> | ||
| /// Refreshes the conditions. Works for both ConditionRefresh and ConditionRefresh2 | ||
| /// </summary> | ||
| public void ConditionRefresh(List<IEventMonitoredItem> monitoredItems, uint monitoredItemId ) |
There was a problem hiding this comment.
This method should be marked as private since it should only be called by ConditionRefresh() or ConditionRefresh2(uint monitoredItemId)
There was a problem hiding this comment.
Updated as requested
| ConditionRefresh(monitoredItems, 0); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
extra empty line here...
|
This pull request fixes 1 alert when merging defd09c into 0c735eb - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging a91db15 into 0c735eb - view on LGTM.com fixed alerts:
|
No description provided.