Skip to content

Firestore: __add__ function for FieldPaths#5149

Merged
tseaver merged 2 commits intogoogleapis:masterfrom
chemelnucfin:firestore_field_path_add
Apr 11, 2018
Merged

Firestore: __add__ function for FieldPaths#5149
tseaver merged 2 commits intogoogleapis:masterfrom
chemelnucfin:firestore_field_path_add

Conversation

@chemelnucfin
Copy link
Contributor

No description provided.

@chemelnucfin chemelnucfin added api: firestore Issues related to the Firestore API. type: cleanup An internal cleanup or hygiene concern. labels Apr 4, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 4, 2018
@chemelnucfin chemelnucfin force-pushed the firestore_field_path_add branch 2 times, most recently from 46dc675 to 060ee8d Compare April 6, 2018 18:56
@tseaver
Copy link
Contributor

tseaver commented Apr 10, 2018

I'm not sure of the utility here. Can @jba or @schmidt-sebastian comment on how / when concatenating FieldPath instances would be used?

@chemelnucfin
Copy link
Contributor Author

chemelnucfin commented Apr 10, 2018

@tseaver For something like this

It's just for convenience, not necessity.

@schmidt-sebastian
Copy link

I don't have any concerns if this internal only. Note that this is called append in the Node/Java clients.

@jba
Copy link
Contributor

jba commented Apr 10, 2018

I have no objections to overloading + to mean append. But in Go I decided to make FieldPaths immutable, because I thought the extra cost was worth avoiding aliasing bugs.

@schmidt-sebastian
Copy link

I have no objections to overloading + to mean append. But in Go I decided to make FieldPaths immutable, because I thought the extra cost was worth avoiding aliasing bugs.

The Node and Java implementations return new copies, so maybe that's the route we should go down everywhere?

@chemelnucfin chemelnucfin force-pushed the firestore_field_path_add branch from 8636b84 to 4f04d0a Compare April 10, 2018 21:18
@jba
Copy link
Contributor

jba commented Apr 10, 2018

LGTM.

@tseaver tseaver merged commit f0607ee into googleapis:master Apr 11, 2018
@chemelnucfin chemelnucfin deleted the firestore_field_path_add branch April 11, 2018 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the Firestore API. cla: yes This human has signed the Contributor License Agreement. type: cleanup An internal cleanup or hygiene concern.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants