Skip to content

Conversation

@derickr
Copy link
Contributor

@derickr derickr commented Jun 21, 2018

No description provided.

@derickr derickr requested a review from jmikola June 21, 2018 15:56
pattern as a document. If specified, then the query system will only consider
plans using the hinted index.
.. versionchanged:: 1.2
Copy link
Member

Choose a reason for hiding this comment

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

The versionchanged macro can be removed here. Since the method itself is versionadded in 1.4, everything is new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

file: apiargs-common-option.yaml
ref: session
post: |
.. versionadded:: 1.3
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

source:
file: apiargs-MongoDBCollection-common-param.yaml
ref: $filter
optional: true
Copy link
Member

Choose a reason for hiding this comment

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

I noted that the CRUD spec states that applications must be required to pass a value, but I'm totally in favor of keeping this optional and defaulting to an empty document. It's consistent with the deviation I made for find().

file: apiargs-common-option.yaml
ref: session
post: |
.. versionadded:: 1.3
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed.


.. phpmethod:: MongoDB\\Collection::estimateDocumentCount()

Gets an estimate of the count of documents in a collection using collection metadata.
Copy link
Member

Choose a reason for hiding this comment

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

I think this reads better (less "of"):

Gets an estimated number of documents in the collection using the collection metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied it out of the CRUD spec... I agree though, so updated.

* @param array $options Command options
* @throws InvalidArgumentException for parameter/option parsing errors
*/
public function __construct($databaseName, $collectionName, array $options = [])
Copy link
Member

Choose a reason for hiding this comment

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

Similar to CountDocuments encapsulating an Aggregate, I think this class an encapsulate a Count operation. We'd then leverage it's existing implementation of Executable and Explainable.

Feel free to lump this in with the ticket for refactoring CountDocuments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created that ticket: https://jira.mongodb.org/browse/PHPLIB-357

throw new UnexpectedValueException('count command did not return a numeric "n" value');
}

return (integer) $result->n;
Copy link
Member

Choose a reason for hiding this comment

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

Another candidate for PHPLIB-356, along with the original Count operation of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

);

case 'estimatedDocumentCount':
return $this->collection->estimatedDocumentCount(
Copy link
Member

Choose a reason for hiding this comment

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

This can probably fit on one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it can!


default:
throw new LogicException('Unsupported operation: ' . $operationName);
throw new LogicException('Unsupported operation: ' . $operation['name']);
Copy link
Member

Choose a reason for hiding this comment

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

Was $operationName undefined? If so, good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was.

* @throws InvalidArgumentException for parameter/option parsing errors
* @throws DriverRuntimeException for other driver errors (e.g. connection errors)
*/
public function countDocuments($filter = [], array $options = [])
Copy link
Member

Choose a reason for hiding this comment

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

As noted elsewhere, this default value on $filter is sufficient to provide a convenience to users. The Operation class can omit the default value.

/reference/method/MongoDBCollection-drop
/reference/method/MongoDBCollection-dropIndex
/reference/method/MongoDBCollection-dropIndexes
/reference/method/MongoDBCollection-estimatedDocumentCount
Copy link
Member

Choose a reason for hiding this comment

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

Building the docs produced the following warning:

source/reference/class/MongoDBCollection.txt:59: WARNING: toctree contains reference to nonexisting document u'reference/method/MongoDBCollection-estimatedDocumentCount'

Looks like a typo in the filename for MongoDBCollection-estimateDocumentCount.txt (missing a "d" before "Document").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the file.

@@ -0,0 +1,56 @@
============================================
MongoDB\\Collection::estimateDocumentCount()
Copy link
Member

Choose a reason for hiding this comment

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

"estimatedDocumentCount"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have this done wrong consistently - mostly.

Definition
----------

.. phpmethod:: MongoDB\\Collection::estimateDocumentCount()
Copy link
Member

Choose a reason for hiding this comment

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

"estimateDocumentCount"

Copy link
Member

@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.

Changes LGTM.

PHPLIB-356 can deal with the integer casts. Thanks for creating PHPLIB-357 to track refactoring.

@derickr derickr force-pushed the PHPLIB-354-count-helpers branch from b75386e to 5464f18 Compare June 22, 2018 14:52
@derickr derickr merged commit 5464f18 into mongodb:master Jun 22, 2018
derickr added a commit that referenced this pull request Jun 22, 2018
@derickr derickr deleted the PHPLIB-354-count-helpers branch August 21, 2018 10:27
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