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

Firestore: __add__ function for FieldPaths #5149

Merged
merged 2 commits into from
Apr 11, 2018

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?

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

None yet

5 participants