-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
* All public of this class are duplicated as instance methods of the | ||
* fluent factory class. | ||
*/ | ||
private const FACTORY_CLASS = Stage::class; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks phpdoc.
} | ||
} | ||
|
||
$staticFactory = TraitType::from(Stage\FactoryTrait::class); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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).
if (! $method->isPublic()) { | ||
continue; | ||
} |
There was a problem hiding this comment.
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(),
);
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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.
I messed with my branches. PR re-opened: #67 |
Related to PHPORM-103
Create a fluent pipeline builder that can be used like this: