Change Summary:
Changed title to be more descriptive
Summary: |
|
---|
Fixed the issue concerning files changed not being present in the review
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+39 -14) |
src/Lunr/Gravity/Database/SQLDMLQueryBuilder.php (Diff revision 2) |
---|
This is technically correct, but not the best solution :)
On the call site
union('SELECT * FROM table', FALSE, TRUE)
doesn't tell you much. But even factoring in named parameters, two arguments still feels odd for alternative options.I.e.
union('SELECT * FROM table', TRUE, TRUE)
makes no sense.So it's better to break the API and change the second parameter to be more inclusive :)
[Update] Changed how the union type is defined so it only takes 2 parameters as before, instead of 3
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+61 -18) |
src/Lunr/Gravity/Database/SQLDMLQueryBuilder.php (Diff revision 3) |
---|
These can be collapsed into a
"UNION $type"
Added missing break to switch case, collapsed $type to avoid repetition, ensured passed parameter is modified to uppercase, wrote unit tests to cover if parameters were passed as booleans.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+102 -17) |
You copy-pasted this, and this isn't wrong, so just consider this an FYI :)
In newer code we do
->with('query')
instead of->with($this->equalTo('query')
and
->willReturn('(query)')
instead of->will($this->returnValue('(query)'))
src/Lunr/Gravity/Database/SQLDMLQueryBuilder.php (Diff revision 4) |
---|
perhaps
NULL
is a better default value?
src/Lunr/Gravity/Database/SQLDMLQueryBuilder.php (Diff revision 4) |
---|
This is good, but it should be in the
sql_compound()
. Otherwise we have to repeat this code for other compound query types likeEXCEPT
orINTERSECT
.(Hint: That's what you'll work on next ;) )
Reworked how method is called using null by default and filtering against ALL or DISTINCT in sql_compound
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+110 -20) |
src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 5) |
---|
I'd just make it a separate parameter. No need to make things more complicated then they need to be :)
Simplified sql_compound by passing 3 parameters, query, type and operator, this should allow for easier and explicit queries, reworked the functions to rely less on string manipulations.
Fixed Style inconsistency
Commit: |
|
||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+115 -29)
|
src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php (Diff revision 7) |
---|
Minor nitpick. We don't need the rtrim here, as the string passed here is always static and never user input.
Changed unit test to pass a third parameter with the operand, removed rtrim as it was passing a white space when the operand was empty. Removed empty verification on sql_compound as it's not necessary because it only needs to verify if it's all or distinct
Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+126 -38)
|
Added branch name
Branch: |
|
||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+117 -29)
|
Reworked how data is passed to the CompoundQuery test and added invalid tests for the CompoundQuery test
Commit: |
|
||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+152 -36)
|
Add dataprovider for union query tests instead of running many individual tests
Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+107 -54)
|
PHPCs fixes
Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+120 -56)
|
src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderTest.php (Diff revision 12) |
---|
spaces after the comma
Fixed comma spacing and alignments
Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 13 (+121 -57)
|