Skip to content
This repository was archived by the owner on Feb 28, 2025. It is now read-only.

Add fluent builder for Laravel #65

Closed
wants to merge 0 commits into from
Closed

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Feb 26, 2024

Related to PHPORM-103

Create a fluent pipeline builder that can be used like this:

        $pipeline = (new FluentFactory())
            ->match(x: Query::eq(1))
            ->project(_id: false, x: true)
            ->sort(x: Sort::Asc)
            ->getPipeline();

@GromNaN GromNaN requested a review from alcaeus February 26, 2024 13:18
@GromNaN GromNaN changed the base branch from 0.1 to 0.2 February 26, 2024 13:19
* All public of this class are duplicated as instance methods of the
* fluent factory class.
*/
private const FACTORY_CLASS = Stage::class;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to get the customized signature of some factory methods, we read the signature from MongoDB\Builder\Stage static methods.

$namespace->addUse(ResolvesToArray::class);
$namespace->addUse(ResolvesToObject::class);
$namespace->addUse(Pipeline::class);
$namespace->addUse(AccumulatorInterface::class);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This long list should be read from the source code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the generated code include fully qualified names without these use statements, or would the code break? IMO, in generated code it's fine to not rely on use statements, alternatively we could always generate the code with fully qualified names and then let phpcbf fix whatever it can (without actually checking the code against our styleguide so that manual fixes need not to be applied).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks phpdoc.

@GromNaN GromNaN marked this pull request as ready for review March 12, 2024 09:05
}
}

$staticFactory = TraitType::from(Stage\FactoryTrait::class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the factory class was extracted to a private constant, but this trait was not. Should this be extracted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to use reflection to get the trait.

$namespace->addUse(ResolvesToArray::class);
$namespace->addUse(ResolvesToObject::class);
$namespace->addUse(Pipeline::class);
$namespace->addUse(AccumulatorInterface::class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the generated code include fully qualified names without these use statements, or would the code break? IMO, in generated code it's fine to not rely on use statements, alternatively we could always generate the code with fully qualified names and then let phpcbf fix whatever it can (without actually checking the code against our styleguide so that manual fixes need not to be applied).

Comment on lines 102 to 104
if (! $method->isPublic()) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this check into addMethod would allow you to simplify the entire block:

array_map(
    fn ($method) => $this->addMethod($method, $namespace, $class),
    $staticFactory->getMethods(),
);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I move the isPublic check but keep the foreach.

->getPipeline();

$this->assertInstanceof(Pipeline::class, $pipeline);
$this->assertCount(3, $pipeline->getIterator());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add an assertion to ensure that the correct stages are added? As is we're only checking that three stages are added, but not what is actually being added.
A possible alternative would be to duplicate the existing stage tests and creating them with the fluent builder instead of the static one, e.g.:

public function testEqualityMatch(): void
{
    $pipeline = (new FluentFactory())
        ->match(author: 'dave'),
        ->getPipeline();

    $this->assertSamePipeline(Pipelines::MatchEqualityMatch, $pipeline);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think replicating all tests is interesting. This single test can cover all specific cases.

@GromNaN
Copy link
Member Author

GromNaN commented Mar 12, 2024

I messed with my branches. PR re-opened: #67

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants