RM-27055 added Queue methods: GetItems, GetItem, Search, Clear#163
RM-27055 added Queue methods: GetItems, GetItem, Search, Clear#163nikepan wants to merge 2 commits intoWorkiva:masterfrom
Conversation
RavenNumber of Findings: 0 |
nikepan
changed the title
|
+1 |
1 similar comment
|
+1 |
|
You'll need to pull in master to get the build to pass @nikepan |
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on HipChat: InfoSec Forum. |
|
Updated |
rmconsole-wf
changed the title
|
+1 |
|
+1 |
|
|
||
| returnItems := make([]interface{}, 0, length) | ||
| for _, item := range *items { | ||
| if !checker(item) { |
There was a problem hiding this comment.
to me it seems unintuitive that this would return objects that do not pass checker, I'd expect the opposite behavior
There was a problem hiding this comment.
Agreed. This logic should probably be inverted and the name changed to something like matcher.
| return nil, false | ||
| } | ||
|
|
||
| // GetItems returns items in this queue. |
There was a problem hiding this comment.
Comment doesn't seem to define the exact behavior.
| return nil | ||
| } | ||
|
|
||
| q.lock.Lock() |
There was a problem hiding this comment.
I realize that this is an ancient PR, but it might be better to do a single deferred unlock here as is done elsewhere in this file.
| result = q.GetItems() | ||
| assert.Len(t, result, 0) | ||
| } | ||
|
|
There was a problem hiding this comment.
Thank you for adding/expanding tests.
|
As I have time I am working through this backlog of PRs. Is this still work you wish to move forward with? If so, there are a few items we need to address. If not, I will close this out but possibly incorporate some of your ideas in a future PR. Please let us know. Thank you for your contribution here. |
8 participants
Some additional methods for working with queue.
Thats useful, when you need interact with queue items (send stop signal and clear queue)