Conversation
bshaffer
left a comment
There was a problem hiding this comment.
Second pass (all minor stuff)
bshaffer
left a comment
There was a problem hiding this comment.
Looks great! Just a few suggestions for options validation (not required), and also please update the PHPDoc for $options in Operation::allocateIds to list the supported options
…bility with previous versions
c7618b4 to
9ebfdc5
Compare
1af2b1d to
9b1f5a2
Compare
e4518f0 to
57040d2
Compare
bshaffer
left a comment
There was a problem hiding this comment.
I think we should go through each handwritten class file in src and promote the properties to the __construct constructor and give them types.
| $readOptions = $this->createReadOptions($this->pluckArray( | ||
| [ | ||
| 'readConsistency', | ||
| 'transaction', | ||
| 'readTime' | ||
| ], | ||
| $options, | ||
| false | ||
| )); |
There was a problem hiding this comment.
the third option here doesn't exist! That's only required for pluck, not pluckArray.
also, we can format this to be a bit less verbose (IMHO)
| $readOptions = $this->createReadOptions($this->pluckArray( | |
| [ | |
| 'readConsistency', | |
| 'transaction', | |
| 'readTime' | |
| ], | |
| $options, | |
| false | |
| )); | |
| $readOptions = $this->createReadOptions($this->pluckArray( | |
| ['readConsistency', 'transaction', 'readTime'], | |
| $options, | |
| )); |
| $encode, | ||
| $returnInt64AsObject, | ||
| $connectionType = 'grpc' | ||
| $returnInt64AsObject |
There was a problem hiding this comment.
For these we could use inline constructor properties:
public function __construct(
private string $projectId,
private bool $encode,
private bool $returnInt64AsObject,
) {| $projectId, | ||
| $namespaceId, | ||
| EntityMapper $entityMapper, | ||
| $databaseId = '' |
There was a problem hiding this comment.
here as well - all of these arguments can be defined as properties in the constructor:
public function __construct(
private DatastoreClient $gapicClient,
private string $projectId,
private string $namespaceId,
private EntityMapper $entityMapper,
private string $databaseId = ''
) {There was a problem hiding this comment.
both $entityMapper and $query can be given types and defined in the constructor
There was a problem hiding this comment.
properties $operation, $transactionId, and $projectId should be typed and defined in the constructor
688a592 to
103ba8d
Compare
This is an effort to update the Datastore library to start using a standardized way. This consists in removing the
ConnectionInterfaceand replacing it for our generated client.b/338660654
BREAKING_CHANGE_REASON=removing connection classes in favor of generated client, which will be breaking. We are incrementing a new major version