-
Notifications
You must be signed in to change notification settings - Fork 320
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
feat(delayedack): Add type filter for delayedack query #860
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #860 +/- ##
==========================================
- Coverage 29.13% 29.11% -0.03%
==========================================
Files 242 243 +1
Lines 33644 33729 +85
==========================================
+ Hits 9803 9819 +16
- Misses 22385 22453 +68
- Partials 1456 1457 +1 ☔ View full report in Codecov by Sentry. |
} | ||
|
||
if len(args) > 2 { | ||
typeStr := strings.ToUpper(args[2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand in the code, in order to filter by type, we need to filter it by status first. We should make the code separate between status and type. Let do them same as this, check if the first argument start with ON_ ... . If yes then it should be filtered by type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in packets-by-rollapp
the status and type are optional filters. In packets-by-status
the type is optional. But you still have packets-by-type
so you can use that to filter by type alone, granted the status will be PENDING by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is like, when you assert like that, the filter must always be status first, and then after that it can be type. So it's kinda annoying if I want to filter just by type
@zale144 I would avoid the refactor of moving types to the types common for 2 reasons:
if there is a way to avoid it and minimize changes would be preferred imo unless I'm missing something. |
Description
Add delayedack filter by type
Closes #850
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
PR review checkboxes:
I have...
Unreleased
section inCHANGELOG.md
godoc
commentsSDK Checklist
map
time.Now()
sendCoin
and notSendCoins
Full security checklist here
----;
For Reviewer:
---;
After reviewer approval: