List workitems #120
List workitems #120
Conversation
|
Apart from a few questions that I have and some missing comments that would increase the readers understanding while reviewing. Great job Thomas! This is quite some complex stuff! This is one moment where I wish Go would not only have duck-typing but and explicit Reviewed 11 of 11 files at r1. workitem.go, line 78 [r1] (raw file):
Just a small issue: Can The criteria/criteria.go, line 153 [r1] (raw file):
I wonder if I can find out how operator precedence is handled here ;) criteria/iterator.go, line 9 [r1] (raw file):
It might be obvious but I think it's worth mentioning that criteria/iterator_test.go, line 13 [r1] (raw file):
Would be nice if there was a comment about depth-first and left-to-right-order. But no real issue. As somebody reviewing the test it's nice to know what somebody expects, you know? criteria/iterator_test.go, line 32 [r1] (raw file):
So models/expression_compiler.go, line 15 [r1] (raw file):
models/expression_compiler.go, line 74 [r1] (raw file):
Ah, so here operator precedence is implicitly modeled through parenthesis, right? I mean without parenthesis we would need a precedence defined but not with I wonder if a check needs to be implemented here: if left != nil && right != nil {
lstr, err := left.(string)
if err != nil {
return err
}
rstr, err := right.(string)
if err != nil {
return err
}
return "(" + lstr + " " + op + " " + rstr + ")"
}
return nilCan one possible inject a models/expression_compiler.go, line 90 [r1] (raw file):
This one I don't get: models/expression_compiler.go, line 99 [r1] (raw file):
A comment would be nice that says that models/expression_compiler.go, line 108 [r1] (raw file):
models/expression_compiler.go, line 124 [r1] (raw file):
models/expression_compiler_test.go, line 7 [r1] (raw file):
I'm no big fan of dot imports but I understand why one wants to do it here. Let's just not use it in other code that we write ourselves. models/expression_compiler_test.go, line 16 [r1] (raw file):
Damn it, I still don't get the models/expression_compiler_test.go, line 27 [r1] (raw file):
So this is where the slice of errors gets used, right? Is it used somewhere else outside of tests? I didn't notice it. models/gorm_repository.go, line 178 [r1] (raw file):
If negative values for What if query/simple/simple_parser.go, line 12 [r1] (raw file):
So the Literal(true) is always included? I only see the Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 16 unresolved discussions. workitem.go, line 78 [r1] (raw file):
|
|
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed. workitem.go, line 78 [r1] (raw file):
|
tsmaeder
force-pushed
the
list-workitems
branch
from
03a1430
to
b3b5dc5
Compare
|
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed. workitem.go, line 78 [r1] (raw file):
|
|
Review status: 6 of 11 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. models/expression_compiler.go, line 90 [r1] (raw file):
|
|
Reviewed 5 of 5 files at r2. Comments from Reviewable |
|
Comments from Reviewable |
tsmaeder
force-pushed
the
list-workitems
branch
from
b3b5dc5
to
aeeb28c
Compare
|
Review status: 6 of 12 files reviewed at latest revision, all discussions resolved. models/gorm_repository.go, line 178 [r1] (raw file):
|
|
Reviewed 6 of 6 files at r3. models/expression_compiler.go, line 70 [r3] (raw file):
|
|
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. models/expression_compiler.go, line 70 [r3] (raw file):
|
tsmaeder
force-pushed
the
list-workitems
branch
from
aeeb28c
to
4521f43
Compare
|
Reviewed 1 of 1 files at r4. Comments from Reviewable |
| } | ||
|
|
||
| if len(result) != 1 { | ||
| t.Errorf("unexpected length, is %d but should be %d", 1, len(result)) |
There was a problem hiding this comment.
I just found that the arguments to the Errorf call should be switched, right?
t.Errorf("unexpected length, is %d but should be %d", 1, len(result))should be
t.Errorf("unexpected length, is %d but should be %d", len(result), 1)
This pull request fixes #101
The PR implements a very simple "query by example" functionality where you can add two parameters to the url "api/workitem"
Example:
http://localhost:8080/api/workitem?filter={"system.owner": "tmaeder","Name": "First","Type": "1"}&page=1,2
The PR has a query parser, which produces expression trees, which will in turn be compiled for use with Gorm. While the query system architecture is in place, only a small amount of expressions is implemented (for example, only "=", no other comparisons). The intention is to extend the system as needed.
This change is