-
Notifications
You must be signed in to change notification settings - Fork 187
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
add file type filter chip #7602
Conversation
b63e5d7
to
dc81e6a
Compare
the tests fail locally, let me know if i can help |
efab378
to
d829aef
Compare
f50f4a6
to
263b6d8
Compare
0ab54b5
to
a408cbf
Compare
7800dcb
to
defdcaa
Compare
from testing it on web i can say it works perfect, just cant approve the PR because i think someone from the ocis team has more knowledge of the topic |
if left, ok := ln.(*bleveQuery.DisjunctionQuery); ok { | ||
// if both are DisjunctionQuery then merge | ||
if right, ok := rn.(*bleveQuery.DisjunctionQuery); ok { | ||
left.AddQuery(right.Disjuncts...) | ||
return left | ||
} | ||
left.AddQuery(rn) | ||
return left | ||
} | ||
if _, ok := ln.(*bleveQuery.ConjunctionQuery); !ok { | ||
if right, ok := rn.(*bleveQuery.DisjunctionQuery); ok { | ||
left := bleveQuery.NewDisjunctionQuery([]bleveQuery.Query{ln}) | ||
left.AddQuery(right.Disjuncts...) | ||
return left | ||
} | ||
} |
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.
We are type asserting ln
and rn
multiple times here. Can't we solve that with a typeswitch? Like
right, ok := rn.(*bleveQuery.DisjunctionQuery)
switch left := ln.(type) {
default:
if ok {
left := bleveQuery.NewDisjunctionQuery([]bleveQuery.Query{ln})
left.AddQuery(right.Disjuncts...)
return left
}
fallthrough
case *bleveQuery.ConjunctionQuery:
return bleveQuery.NewDisjunctionQuery([]bleveQuery.Query{ln, rn})
case *bleveQuery.DisjunctionQuery:
if ok {
left.AddQuery(right.Disjunkts)
} else {
left.AddQuery(rn)
}
return left
}
Or similar. But we want to make the type assertions only once
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.
The fallthrough doesn't work in this way.
I can rewrite but It became badly readable, sounds to me
right, ok := rn.(*bleveQuery.DisjunctionQuery)
switch left := ln.(type) {
case *bleveQuery.DisjunctionQuery:
if ok {
left.AddQuery(right.Disjuncts...)
} else {
left.AddQuery(rn)
}
return left
case *bleveQuery.ConjunctionQuery:
return bleveQuery.NewDisjunctionQuery([]bleveQuery.Query{ln, rn})
default:
if ok {
left := bleveQuery.NewDisjunctionQuery([]bleveQuery.Query{ln})
left.AddQuery(right.Disjuncts...)
return left
} else {
return bleveQuery.NewDisjunctionQuery([]bleveQuery.Query{ln, rn})
}
}
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.
Imo better readable than the double type assertion. If you ask me I'd go with second solution. But up to you in the end...
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.
You could skip the last else
though 😉
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.
reworked
Kudos, SonarCloud Quality Gate passed! |
* add file type filter chip * changed the keyword * use reva issue-7432-replace * changed the filter name. fix types * rework mapBinary --------- Co-authored-by: Roman Perekhod <rperekhod@owncloud.com>
Description
Related Issue
Motivation and Context
How Has This Been Tested?
The request example
Screenshots (if appropriate):
Types of changes
Checklist: