-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix station filter logic for nodes having no children. #232
Conversation
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.
Just some variable names that are hard to understand.
cmd/fdsn-ws/fdsn_station.go
Outdated
nNet := len(r.Network) | ||
r.Network = ns | ||
|
||
if len(ns) == 0 { | ||
// No result ( no "Network" node ) | ||
if nNet != 0 && len(ns) == 0 { | ||
// this means the query has network parameter but idn't match one, we want to give a no data | ||
return false | ||
} |
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.
nNet
is strange.
Can ns
be networks
? or better yet filteredNetworks
.
Would a defer r.Network = ns
help, just after for loop? Also for station/channel sections.
Comment missing a d
in didn't
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.
There's still trimLevel
need to operate on the result slice. So can't move the assignment to defer.
d754a0b
to
0cc80d8
Compare
@CallumNZ I found that the logic was incorrect. Could you please review my new update? Thanks |
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.
Looks good.
for _, p := range matchedParams { | ||
if p.StationReg == nil && p.ChannelReg == nil && p.LocationReg == nil { | ||
return true | ||
if p.StationReg != nil && !contains(p.StationReg, REGEX_ANYTHING) { |
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 does the regex check add?
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.
See Jerome's comment regarding issue with WTSZ here https://github.com/GeoNet/tickets/issues/12671#issuecomment-1452655370
Short answer is : "*" includes "0 results".
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.
Looks good
As requested in https://github.com/GeoNet/tickets/issues/12671.
Also, text output will be in sync with XML output GeoNet/help#103.
Previously we deliberately make it not exists, so the fix is remove those extras.