[Lunr] Gravity: Added support to call Union Distinct explicitly

Review Request #822 — Created April 6, 2022 and submitted

d.cova
Lunr
gravity-union
825
ffbccae...
lunr

[Update] Added support in Gravity to call Union Distinct explicitly

Unit tests to cover testUnionDistinct

  • 0
  • 0
  • 20
  • 0
  • 20
Description From Last Updated
d.cova
smillernl
smillernl
  1. 
      
  2. Is this still a work in progress? Because I think you'll need a little more to make this feature work.

  3. 
      
d.cova
pprkut
  1. 
      
  2. 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 :)

  3. whitespace error :)

  4. 
      
d.cova
smillernl
  1. 
      
  2. There should probably be a test for the old value too (boolean).

  3. src/Lunr/Gravity/Database/SQLDMLQueryBuilder.php (Diff revision 3)
     
     
     
     
     
     
     

    These can be collapsed into a "UNION $type"

  4. 
      
d.cova
pprkut
  1. 
      
  2. 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)'))

  3. perhaps NULL is a better default value?

  4. 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 like EXCEPT or INTERSECT.

    (Hint: That's what you'll work on next ;) )

  5. 
      
d.cova
pprkut
  1. 
      
  2. I'd just make it a separate parameter. No need to make things more complicated then they need to be :)

  3. 
      
d.cova
pprkut
d.cova
pprkut
  1. 
      
    1. Thanks! There was actually something else that was causing that whitespace and now it clicked

  2. Minor nitpick. We don't need the rtrim here, as the string passed here is always static and never user input.

  3. 
      
d.cova
pprkut
  1. 
      
  2. Branch information is missing :)

  3. Diff contains unrelated changes :)

  4. 
      
d.cova
pprkut
  1. 
      
  2. src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    It's better if you split $types into 2 separate variables here (already at the input) and adjust the data provider accordingly.

  3. We also now need a test for unsupported operators

  4. 
      
d.cova
d.cova
pprkut
  1. 
      
  2. src/Lunr/Gravity/Database/MySQL/Tests/MySQLSimpleDMLQueryBuilderSelectTest.php (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    All these tests can be combined, with a data provider giving the individual test cases.

  3. 
      
d.cova
d.cova
pprkut
  1. 
      
  2. Probably good to run phpcs on this ;)

  3. src/Lunr/Gravity/Database/MySQL/Tests/MySQLSimpleDMLQueryBuilderSelectTest.php (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Totally nitpicking here :)

    /**
     * Test union()
     *
     * @param ...
     *
     * @dataProvider unionOperatorProvider
     * @covers       Lunr\...
     */
    public function testUnion($operator): void
    

    So, missing @param, correct alignment as spacing in the phpdoc, and singular param name.

  4. 
      
d.cova
pprkut
d.cova
pprkut
Review request changed

Status: Closed (submitted)

Change Summary:

Merged into master

Loading...