Skip to content

FieldValue changes for transform_operation #8017

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

Merged
merged 2 commits into from
May 3, 2021

Conversation

schmidt-sebastian
Copy link
Contributor

These are two files from #7904 that should be able to be reviewed relatively independently. Most of the missing functions are in review here: #7991 (such as IsArray.

This PR changes all FieldTransforms to use Protobuf directly. It follows the code on Android: https://github.com/firebase/firebase-android-sdk/tree/master/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation

@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

} else {
// coerce to empty array.
// coerce to empty array.;
Copy link
Contributor

Choose a reason for hiding this comment

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

ultranit: This semicolon addition to the end of the line appears to be a typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}
return FieldValue::FromArray(std::move(result));

google_firestore_v1_Value result{};
Copy link
Contributor

Choose a reason for hiding this comment

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

ultranit: Can the empty initializer list just be omitted? Its presence throws me off a bit because I'm wondering if it is there for some specific reason. Also on lines 393 and 418.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The empty initializer zeroes-out the values. This is a struct, so there is no default constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh makes sense.

@schmidt-sebastian schmidt-sebastian merged commit e465253 into mrschmidt/mutabledocuments May 3, 2021
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/transform branch May 3, 2021 15:25
@firebase firebase locked and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants