Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Agg builder blog post #2

Draft
wants to merge 5 commits into
base: v2.x
Choose a base branch
from
Draft

Conversation

alcaeus
Copy link
Owner

@alcaeus alcaeus commented Oct 23, 2024

Please ignore the test files - only article.md needs review.

tests/AggregationBlogPost/article.md Outdated Show resolved Hide resolved
tests/AggregationBlogPost/article.md Outdated Show resolved Hide resolved
tests/AggregationBlogPost/article.md Outdated Show resolved Hide resolved
tests/AggregationBlogPost/article.md Outdated Show resolved Hide resolved
tests/AggregationBlogPost/article.md Outdated Show resolved Hide resolved
tests/AggregationBlogPost/article.md Outdated Show resolved Hide resolved
tests/AggregationBlogPost/article.md Outdated Show resolved Hide resolved
operators or even stages, so we wanted to make sure that we can easily expand
the builder.

We could try to rely on generative AI to help us with this, but this only goes
Copy link

Choose a reason for hiding this comment

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

Please tell me "generative AI" here was intended as an SEO play for the blog post. Did we, at any point, actually consider using it for this project? IIRC, it was always defining the aggregation syntax in YAML and employing code generation.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes and no. I may or may not have used GitHub CoPilot to protect my sanity in one or more larger pull requests introducing aggregation builder functionality in the Doctrine ODM recently.

SEO is a nice benefit that I'll happily take.

tests/AggregationBlogPost/article.md Outdated Show resolved Hide resolved
Comment on lines 710 to 713
the builder and feel what it's like. To top it all off, we could add this code
to the server documentation, similar to how we add other language-specific code
snippets. We're not quite there yet, but we'd love to include this in the
documentation.
Copy link

Choose a reason for hiding this comment

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

Ultimately, we love to add this builder code to the server documentation, similar to how we add other language-specific code snippets, but we're not quite there yet.

See if that reads better. I don't think this idea warrants more than a single sentence.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, much better 👍

@alcaeus alcaeus requested a review from jmikola October 24, 2024 11:30
Copy link

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

A few more optional suggestions. Take what you like.

help beyond syntax highlighting. Pair that with a few levels of nesting, and
you've got yourself the kind of code that you can write, but not read. We could
start off by refactoring the code, but instead let's try to move away from array
structures and use a better solution.
Copy link

Choose a reason for hiding this comment

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

Suggested change
structures and use a better solution.
structures and towards a better solution.

"Towards" balances "move away" nicely.

refactor this code, but before we get into that, we want to move away from these
array structures.
is that the only way to express the aggregation framework domain-specific
language (DSL) is through untyped arrays, and any PHP editor can't provide much
Copy link

Choose a reason for hiding this comment

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

Suggested change
language (DSL) is through untyped arrays, and any PHP editor can't provide much
language (DSL) is through untyped arrays, for which a PHP editor provides little

See if that reads better.

Comment on lines +746 to +748
We also decided against this option, partly because the underscore marks (or at
least used to mark) private functions or methods, but also because the
underscore would break alphabetical sorting in code completion.
Copy link

Choose a reason for hiding this comment

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

Suggested change
We also decided against this option, partly because the underscore marks (or at
least used to mark) private functions or methods, but also because the
underscore would break alphabetical sorting in code completion.
We also decided against this option, partly because underscores have
historically been used to mark private methods and properties, but also
because the underscore would break alphabetical sorting in code completion.

underscore would break alphabetical sorting in code completion.

A less-than-serious approach was to use emojis, which I learned was valid in
PHP:
Copy link

Choose a reason for hiding this comment

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

If you want to be especially evil: https://en.wikipedia.org/wiki/Zero-width_space

);
```

Again, PHP's use of `$` to mark variables plays nice with aggregation pipeline
Copy link

Choose a reason for hiding this comment

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

Suggested change
Again, PHP's use of `$` to mark variables plays nice with aggregation pipeline
Again, PHP's use of `$` to mark variables plays nice with the aggregation framework

This is the second time I noticed you refer to "aggregation pipeline" as you would a proper noun. I think "the" is necessary here as it's a thing and not a formal name.

Also changed to "framework" because I don't consider the pipeline as a logical owner of stages and operators. It's just another component where those get used.

Comment on lines +803 to +804
We have not implemented this for now, but if you prefer this syntax to static
methods, or if you have alternative suggestions, please let us know about them!
Copy link

Choose a reason for hiding this comment

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

Suggested change
We have not implemented this for now, but if you prefer this syntax to static
methods, or if you have alternative suggestions, please let us know about them!
We have decided not implemented this for now, but if you prefer this syntax to static
methods, or if you have alternative suggestions, please let us know about them!

I felt like a word was missing here.

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

Successfully merging this pull request may close these issues.

2 participants