Field path class #4392

chemelnucfin · 2017-11-14T07:08:02Z

@jba please see if this is the desired behavior you wanted or feel free to give me any comments.

jba · 2017-11-14T18:25:20Z

firestore/google/cloud/firestore_v1beta1/_helpers.py

+        invalid_characters = '~*/[]'
+        if len(parts) == 1:
+            parts = parts[0]
+        if isinstance(parts, basestring):


firestore/google/cloud/firestore_v1beta1/_helpers.py

+        for part in parts:
+            if (not part
+                or not isinstance(part, basestring)
+                or part in invalid_characters):


firestore/google/cloud/firestore_v1beta1/_helpers.py

+                    else:
+                        part_ans += letter
+                ans.append('`' + part_ans  + '`')
+        return '.'.join(ans)


firestore/tests/unit/test__helpers.py

+
+    def test_empty_string_inside_string_fails(self):
+        with self.assertRaises(ValueError):
+            field_path = self._make_one('a..b')


firestore/google/cloud/firestore_v1beta1/_helpers.py

+
+    def __init__(self, *parts):
+        invalid_characters = '~*/[]'
+        if len(parts) == 1:


firestore/google/cloud/firestore_v1beta1/_helpers.py

+            if (not part
+                or not isinstance(part, basestring)
+                or part in invalid_characters):
+                raise ValueError


firestore/google/cloud/firestore_v1beta1/_helpers.py

+
+        Returns: string representation of the path stored within
+        """
+        pattern = re.compile(r'[A-Za-z_][A-Za-z_0-9]*')


firestore/google/cloud/firestore_v1beta1/_helpers.py

+            if match:
+                ans.append(part)
+            else:
+                part_ans = ''


googlebot · 2017-11-15T17:26:08Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

chemelnucfin · 2017-11-15T17:49:30Z

@jba @schmidt-sebastian PTAL.

I used the string replace method like node. Please check my tests to see that they are still the desired behavior.

In addition, what about unicode?

I will work on the update and query methods in a separate PR.

chemelnucfin · 2017-11-15T18:40:26Z

I will wait on updating update and query methods until I get more confirmation about the unicode issue.

chemelnucfin · 2017-11-20T02:10:39Z

@jba @schmidt-sebastian
So I'm looking through the different languages, and I see different things for each language. For example, the function api_repr that I was implementing is called
toServiceFieldPathComponent in Go and C#,
but canonicalString in Java and Node

I understand each language has different quirks and that each user would probably not be using cross language. However, shouldn't something like this be consistent across all languages?

Also, according to the docs that was here:
https://cloud.google.com/firestore/docs/reference/rest/v1beta1/Document
it doesn't seem like unicode is allowed, but is there a reason for that?

Thanks.

jba · 2017-11-20T16:26:18Z

The doc is now at https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.Document and it explicitly allows unicode keys (it refers to UTF-8).

It's not important that the names of unexposed functions are the same across languages. We only care about the exposed surface and the behavior.

firestore/google/cloud/firestore_v1beta1/_helpers.py

+
+    @classmethod
+    def from_string(cls, string):
+        """ Creates a FieldPath with a string representation.


firestore/google/cloud/firestore_v1beta1/_helpers.py

+        self.parts = tuple(parts)
+
+    @classmethod
+    def from_string(cls, string):


firestore/google/cloud/firestore_v1beta1/_helpers.py

+        return FieldPath(*string)
+
+    def to_api_repr(self):
+        """ Returns string representation of the FieldPath


firestore/google/cloud/firestore_v1beta1/_helpers.py

+        parts: (one or more strings)
+            Indicating path of the key to be used.
+    """
+    pattern = re.compile(r'[A-Za-z_][A-Za-z_0-9]*')


firestore/tests/unit/test__helpers.py

+        field_path = self._make_one(parts)
+        self.assertEqual(r'`\\\\`', field_path.to_api_repr())
+
+    def test_to_api_repr_underscore_valid(self):


firestore/tests/unit/test__helpers.py

+        with self.assertRaises(ValueError):        
+            field_path = self._make_one(parts)
+
+    def test_key(self):


firestore/google/cloud/firestore_v1beta1/_helpers.py

+    def __hash__(self):
+        return hash(self.to_api_repr())
+
+    def __eq__(self, other):


firestore/google/cloud/firestore_v1beta1/_helpers.py

+        """
+        invalid_characters = '~*/[]'
+        string = string.split('.')
+        for part in string:


chemelnucfin · 2017-12-21T20:09:04Z

@schmidt-sebastian I moved the validation and did the empty string check in the constructor. Please take a look at my 3rd review changes (4th commit) and see if that's what you meant. Line 135 did not check for an empty string before.

schmidt-sebastian · 2017-12-21T20:53:44Z

@schmidt-sebastian I moved the validation and did the empty string check in the constructor. Please take a look at my 3rd review changes (4th commit) and see if that's what you meant. Line 135 did not check for an empty string before.

Looks good to me. Thanks.

chemelnucfin · 2018-01-10T22:08:59Z

@dhermes?

dhermes · 2018-01-10T22:17:30Z

Sorry I don't have cycles for this right now. @tseaver can you tap in? (In particular, we want to avoid re-inventing the wheel for code that already does this.)

chemelnucfin · 2018-01-10T22:23:20Z

@dhermes it's ok. I am just trying to clean out some issues/prs.

chemelnucfin · 2018-03-06T04:20:52Z

I'm going to go ahead and merge this in. Let me know if there are any issues.

chemelnucfin requested a review from lukesneeringer as a code owner November 14, 2017 07:08

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 14, 2017

chemelnucfin changed the title ~~#4378 - Field Path~~ Firestore: Field Path Nov 14, 2017

schmidt-sebastian reviewed Nov 14, 2017

View reviewed changes

firestore/google/cloud/firestore_v1beta1/_helpers.py Outdated

invalid_characters = '~*/[]'

if len(parts) == 1:

parts = parts[0]

if isinstance(parts, basestring):

This comment was marked as spam.

Sign in to view

schmidt-sebastian reviewed Nov 14, 2017

View reviewed changes

firestore/google/cloud/firestore_v1beta1/_helpers.py Outdated

else:

part_ans += letter

ans.append('`' + part_ans + '`')

return '.'.join(ans)

This comment was marked as spam.

Sign in to view

schmidt-sebastian reviewed Nov 14, 2017

View reviewed changes

jba suggested changes Nov 14, 2017

View reviewed changes

chemelnucfin added api: spanner Issues related to the Spanner API. api: firestore Issues related to the Firestore API. and removed api: spanner Issues related to the Spanner API. labels Nov 14, 2017

chemelnucfin force-pushed the 4378-fieldpathclass branch from ed331d7 to 1592e66 Compare November 15, 2017 07:31

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Nov 15, 2017

chemelnucfin force-pushed the 4378-fieldpathclass branch 3 times, most recently from dcdfd4e to 6425b42 Compare November 15, 2017 17:36

chemelnucfin added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Nov 15, 2017

chemelnucfin force-pushed the 4378-fieldpathclass branch 4 times, most recently from 1b4a259 to dd1ff57 Compare November 20, 2017 02:05

jba suggested changes Nov 20, 2017

View reviewed changes

chemelnucfin mentioned this pull request Dec 20, 2017

firestore: add a FieldPath class #4378

Closed

schmidt-sebastian approved these changes Dec 21, 2017

View reviewed changes

firestore/google/cloud/firestore_v1beta1/_helpers.py Outdated

"""

invalid_characters = '~*/[]'

string = string.split('.')

for part in string:

This comment was marked as spam.

Sign in to view

chemelnucfin force-pushed the 4378-fieldpathclass branch 6 times, most recently from 0d0e214 to 0a379dd Compare December 21, 2017 19:59

chemelnucfin force-pushed the 4378-fieldpathclass branch from 0a379dd to 9fb3721 Compare December 21, 2017 20:18

chemelnucfin assigned dhermes and chemelnucfin Jan 15, 2018

chemelnucfin added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Jan 15, 2018

chemelnucfin changed the title ~~Firestore: Field path~~ Field path class Feb 20, 2018

chemelnucfin added 3 commits March 5, 2018 17:11

googleapis#4378 - Field Path

0f9f928

review changes

09e834d

2nd review changes

1843740

chemelnucfin force-pushed the 4378-fieldpathclass branch from 9fb3721 to ed6e22e Compare March 6, 2018 01:11

3rd review changes

3d2631b

chemelnucfin force-pushed the 4378-fieldpathclass branch from ed6e22e to 3d2631b Compare March 6, 2018 01:40

chemelnucfin merged commit 96309fc into googleapis:master Mar 6, 2018

chemelnucfin deleted the 4378-fieldpathclass branch March 26, 2018 23:36

theacodes unassigned dhermes Sep 28, 2018

Field path class #4392

Field path class #4392

Conversation

chemelnucfin commented Nov 14, 2017

jba commented Nov 14, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

googlebot commented Nov 15, 2017

chemelnucfin commented Nov 15, 2017

chemelnucfin commented Nov 15, 2017

chemelnucfin commented Nov 20, 2017 • edited Loading

jba commented Nov 20, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

chemelnucfin commented Dec 21, 2017

schmidt-sebastian commented Dec 21, 2017

chemelnucfin commented Jan 10, 2018

dhermes commented Jan 10, 2018

chemelnucfin commented Jan 10, 2018

chemelnucfin commented Mar 6, 2018

chemelnucfin commented Nov 20, 2017 •

edited

Loading