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

DOCSP-43919: aggregation builder #203

Merged
merged 8 commits into from
Feb 13, 2025

Conversation

rustagir
Copy link
Contributor

@rustagir rustagir commented Feb 10, 2025

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-43919
Staging -

  1. agg page
  2. whats new

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?
  • Are all the links working?
  • Are the facets and meta keywords accurate?

Copy link

netlify bot commented Feb 10, 2025

Deploy Preview for docs-php-library ready!

Name Link
🔨 Latest commit 948fc35
🔍 Latest deploy log https://app.netlify.com/sites/docs-php-library/deploys/67ae250d37e91a000864f31b
😎 Deploy Preview https://deploy-preview-203--docs-php-library.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rustagir rustagir force-pushed the DOCSP-43919-agg-builder branch from a654c02 to 30da9a6 Compare February 10, 2025 18:00
@rustagir rustagir mentioned this pull request Feb 10, 2025
5 tasks
Copy link
Contributor

@lindseymoore lindseymoore 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 formatting and rewording things, but LGTM! Let me know if you want another look

source/aggregation.txt Outdated Show resolved Hide resolved
source/aggregation.txt Outdated Show resolved Hide resolved
#. Create an array to store the pipeline stages

#. For each stage, call an operator method from the ``Stage``
builder class to create that type of aggregation stage
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
builder class to create that type of aggregation stage
builder class to create an aggregation stage of that type

S: Optional rewording, to put emphasis on the stage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reworded

source/includes/aggregation.php Outdated Show resolved Hide resolved
source/whats-new.txt Outdated Show resolved Hide resolved
Comment on lines 90 to 94
- :ref:`php-aggregation-array-api`: Create aggregation pipelines by
passing arrays that specify the aggregation operators and parameters
- :ref:`php-aggregation-builder-api`: Create aggregation pipelines by using native
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Any guidelines on linking to sections on the same page within text and without a proper introduction? Was a little thrown off when this just led me to the section right beneath. If the team usually does this, it's alright. Could remove links if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have many pages that do this (example 1 | 2)

I'm not sure that I get what you mean by

without a proper introduction

source/aggregation.txt Outdated Show resolved Hide resolved
source/aggregation.txt Outdated Show resolved Hide resolved
source/aggregation.txt Outdated Show resolved Hide resolved
@rustagir rustagir force-pushed the DOCSP-43919-agg-builder branch from 0b38fc5 to 70bcf6c Compare February 11, 2025 19:28
@rustagir rustagir requested review from a team and jmikola and removed request for a team February 11, 2025 19:30
source/aggregation.txt Outdated Show resolved Hide resolved
source/aggregation.txt Outdated Show resolved Hide resolved
source/aggregation.txt Outdated Show resolved Hide resolved
source/aggregation.txt Outdated Show resolved Hide resolved
source/aggregation.txt Outdated Show resolved Hide resolved
source/aggregation/atlas-search.txt Show resolved Hide resolved
source/includes/aggregation/aggregation.php Outdated Show resolved Hide resolved
source/whats-new.txt Outdated Show resolved Hide resolved
@rustagir rustagir requested a review from jmikola February 13, 2025 17:03
@rustagir rustagir merged commit bc9c0e2 into mongodb:v1.21 Feb 13, 2025
5 of 7 checks passed
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.

3 participants