[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
There are no open issues
d.cova
smillernl
smillernl
  1. 
      
  2. The issue has been resolved. Show all issues

    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)
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    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. The issue has been resolved. Show all issues

    whitespace error :)

  4. 
      
d.cova
smillernl
  1. 
      
  2. The issue has been resolved. Show all issues

    There should probably be a test for the old value too (boolean).

  3. src/Lunr/Gravity/Database/SQLDMLQueryBuilder.php (Diff revision 3)
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    These can be collapsed into a "UNION $type"

  4. The issue has been resolved. Show all issues

    missing break

  5. 
      
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. The issue has been resolved. Show all issues

    perhaps NULL is a better default value?

  4. src/Lunr/Gravity/Database/SQLDMLQueryBuilder.php (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    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. The issue has been resolved. Show all issues

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

  3. 
      
d.cova
pprkut
  1. 
      
  2. The issue has been resolved. Show all issues

    style ;)

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

  2. The issue has been resolved. Show all issues

    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. The issue has been resolved. Show all issues

    Branch information is missing :)

  3. The issue has been resolved. Show all issues

    Diff contains unrelated changes :)

  4. 
      
d.cova
pprkut
  1. 
      
  2. src/Lunr/Gravity/Database/Tests/DatabaseDMLQueryBuilderQueryPartsTest.php (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

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

  3. The issue has been resolved. Show all issues

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

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

  3. 
      
d.cova
d.cova
pprkut
  1. 
      
  2. The issue has been resolved. Show all issues

    Probably good to run phpcs on this ;)

  3. src/Lunr/Gravity/Database/MySQL/Tests/MySQLSimpleDMLQueryBuilderSelectTest.php (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    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
  1. 
      
  2. The issue has been resolved. Show all issues

    values should be aligned

  3. The issue has been resolved. Show all issues

    spaces after the comma

  4. 
      
d.cova
pprkut
Review request changed

Status: Closed (submitted)

Change Summary:

Merged into master

Loading...