- 
                Notifications
    You must be signed in to change notification settings 
- Fork 184
Update location with updateChildValues #108
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
Conversation
| Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g.  What to do if you already signed the CLAIndividual signers
 Corporate signers
 | 
    
      
        1 similar comment
      
    
  
    | Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g.  What to do if you already signed the CLAIndividual signers
 Corporate signers
 | 
| I signed it! | 
| CLAs look good, thanks! | 
Use updateChildValues (and setPriority) in favor of setValuesAndPriority to avoid overwriting other child values.
| @mcdonamp please review | 
| @RaMin0 @morganchen12 it looks like we don't actually need priority (we store the geohash in  Presumably this is so you can store additional data alongside the GeoFire info? | 
| 
 Yes, it made more sense to save the related info stored in the same node rather than having a parallel node that I have to maintain just to keep the extra info. One other thing I suggest to do is to make the name of the location key (currently   | 
Priority is no longer used. The geohash is stored in and queried by g
| }; | ||
|  | ||
| XCTAssertEqualObjects(snapshot.value, expected); | ||
| XCTAssertEqualObjects([snapshot childSnapshotForPath:@"loc1"].priority, @"7zzzzzzzzz"); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please instead test g rather than priority. We'd like to make sure everything still works :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it already tested on the previous line when comparing the whole value of the snapshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derp, yes.
Use
updateChildValues(andsetPriority) in favor ofsetValuesAndPriorityto avoid overwriting other child values.