-
Notifications
You must be signed in to change notification settings - Fork 4
Use BSON round-trip to normalize pipelines for comparison #13
Conversation
5f6cbd0
to
2b8a6ae
Compare
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.
Two questions you can address on your own but LGTM.
tests/Builder/BuilderEncoderTest.php
Outdated
self::objectify($expected); | ||
// Normalize with BSON round-trip | ||
// BSON Documents doesn't support top-level arrays. | ||
$actual = Document::fromPHP(['root' => $actual])->toRelaxedExtendedJSON(); |
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.
Any reason not to use toCanonicalExtendedJSON()
?
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 must admit that I'm very confused between the 2 methods. toCanonicalExtendedJSON()
is the correct one to use.
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.
Generally, you should stick to canonical mode. Relaxed mode is fine for concise output (e.g. debugging), but canonical is necessary to ensure accurate type comparisons.
|
||
// BSON Documents doesn't support top-level arrays. | ||
$expected = toPHP(fromJSON('{"root":' . $expectedJson . '}'))->root; | ||
$expected = '{"pipeline":' . $expectedJson . '}'; |
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 realize this is outside of the PR diff, but when is a BackedEnum used to store the expected JSON string?
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.
Maybe @GromNaN has some new fancy ENUM ideas? 😆 🙈
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.
It was introduced in #6 to generate test pipelines from config Yaml.
2b8a6ae
to
e44f7d0
Compare
Stage::match( | ||
// sku: \MongoDB\Builder\Query::regex('789$', ''), | ||
sku: new Regex('789$', ''), | ||
sku: Query::regex('789$', ''), |
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.
Looks like comparing JSON allows to fix this issue: #6 (comment)
Use the driver feature to normalize the aggregation pipeline in PHP.
Diffs are easier to read when using JSON comparison.
Part of PHPLIB-1291
Related to #8