Skip to content
This repository has been archived by the owner on Feb 5, 2024. It is now read-only.

Add method bulkGet #91

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

sidneycorreia
Copy link

This method can be called to query several documents in bulk. It is well suited for fetching a specific revision of documents, as replicators do for example, or for getting revision history.

*
* @throws HTTPException
*
* @return array
Copy link
Member

@greg0ire greg0ire Jan 27, 2020

Choose a reason for hiding this comment

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

The body of the response is an array? What type are the elements in the array exactly?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Your link is broken

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok so it should also be

Suggested change
* @return array
* @return array<int, array<string, string>>

correct?

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

Thank you. Please add tests for your new added method.

*
* @return array
*/
public function bulkGet(array $docs = [])
Copy link
Member

@greg0ire greg0ire Jan 28, 2020

Choose a reason for hiding this comment

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

Can we add a return type declaration here?

Copy link
Author

@sidneycorreia sidneycorreia Jan 29, 2020

Choose a reason for hiding this comment

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

I think this return type is too complex for method documentation. We can add a link to method's official documentation http://docs.couchdb.org/en/2.2.0/api/database/bulk-api.html#db-bulk-get

Copy link
Member

Choose a reason for hiding this comment

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

Does it return other PHP types than array like the previous return annotation suggests?

Copy link
Member

Choose a reason for hiding this comment

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

I was suggesting this

Suggested change
public function bulkGet(array $docs = [])
public function bulkGet(array $docs = []): array

$docs = [];
$expectedRows = [];
foreach (range(1, 3) as $i) {
list($id, $rev) = $client->postDocument(['foo' => 'bar'.$i]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
list($id, $rev) = $client->postDocument(['foo' => 'bar'.$i]);
[$id, $rev] = $client->postDocument(['foo' => 'bar'.$i]);

Copy link
Member

Choose a reason for hiding this comment

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

The composer constraint is currently "php": ">=5.4", but I assume we are going to have at least 7.2 soon.

@sidneycorreia
Copy link
Author

Thank you. Please add tests for your new added method.

Done

@greg0ire
Copy link
Member

Please associate your email address with your Github account, or change the
email in your commits to an address already associated with it. If you do not
want to expose your personal email address, you may use
[email protected], that way we can still reach you through Github.
Additionally, you will get credit for this contribution on your Github profile.

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

You requested my review so here it is: please address the changes we asked.

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

In addition to what @greg0ire wrote, we need to bump the minimum supported PHP version in composer.json. I suggest 7.2.

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

Successfully merging this pull request may close these issues.

3 participants