feat(datastore): add datastore #35

KevinGDialpad · 2019-06-25T16:40:41Z

This based on the code for #34. I will rebase and remove the WIP after it's merged.

This adds support for the datastore, in a separate project.

The code is pretty much gcloud-aio's datastore

KevinGDialpad

This looks like a lot of code, but the actual code comes directly from gcloud-aio. The wiring is what's important here :)

Also, I didn't add integration tests. I'd like to write some but I can't run them.

Can we add issues to the GitHub project? I'd like to add one for running integration tests (like gcloud-aio's 53) and one for adding integration tests to the datastore and update the README.

.gitignore

.pre-commit-config.py27.yaml

KevinGDialpad · 2019-06-25T16:45:30Z

.pre-commit-config.py27.yaml

@@ -56,9 +52,11 @@ repos:
    rev: v1.4.4
    hooks:
    -   id: autopep8
+        args:
+        - --in-place
+        - --ignore=E124,E226,E24,W50,W690 # E124 conflicts with pylint


I think those kinds of problems are going to be fixed in #34.

This specific one is a conflict between pylint and pep8. See pylint-dev/pylint#747 It seems that this correction is not the right one. But #34 has the same problem so we can fix it there.

Gotcha -- not sure why this hasn't come up for us before, since these hooks are enabled on all of our other repos... what are all of these disables?

E124 in particular I'm ok with disabling -- I think we've generally defaulted to the pylint style anyway.

The other 4 are disabled by default in autopep8, so they were already ignored.

Instead of doing that, we're going to disable bad-continuation in Pylint, because Pylint is wrong in that case. See the last part of the indentation section of PEP-8:

The closing brace/bracket/parenthesis on multiline constructs may either line up under the first non-whitespace character of the last line of list [...]

(the other case is not relevant)

So I think we can remove these 3 lines and ignore bad-continuation in Pylint instead.

cc @gunjanswitchco

I could go either way on accepting the pylint of the autopep8 versions here; I would say pick whichever one our code already conforms to, since in this case I think its more important to have the standard than exactly which one it is. ++ for picking one and rolling with it 👍

README.rst

datastore/README.rst

datastore/gcloud/rest/datastore/datastore.py

KevinGDialpad · 2019-06-25T16:51:18Z

gcloud/rest/auth/__init__.py

@@ -1,7 +1,10 @@
 from pkg_resources import get_distribution


Oops. The auth files are not supposed to be here. Please ignore everything from that point down.

Sure thing -- we'll need to make sure these get removed before merge! We'll get the base auth PR in first.

Yep - I'll rebase onto master once #34 is merged. Since the auth changes will be on master already, they will disappear from this PR.

TheKevJames

Enabled issues on this repo -- not sure why they had been disabled in the first place /shrug

TheKevJames · 2019-06-25T20:08:01Z

.pre-commit-config.py27.yaml

@@ -56,9 +52,11 @@ repos:
    rev: v1.4.4
    hooks:
    -   id: autopep8
+        args:
+        - --in-place
+        - --ignore=E124,E226,E24,W50,W690 # E124 conflicts with pylint


Gotcha -- not sure why this hasn't come up for us before, since these hooks are enabled on all of our other repos... what are all of these disables?

E124 in particular I'm ok with disabling -- I think we've generally defaulted to the pylint style anyway.

TheKevJames · 2019-06-25T20:08:58Z

datastore/LICENSE

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) 2017 TalkIQ


2019, please, since the datastore module is new!

TheKevJames · 2019-06-25T20:15:00Z

datastore/gcloud/rest/datastore/datastore.py

+    import ujson as json
+except ImportError:
+    import json  # type: ignore
+try:


nit: keep the linebreaks here, please -- I'm actually surprised our linting doesn't catch this, there should be two empty lines after all the imports.

TheKevJames · 2019-06-25T20:17:01Z

datastore/gcloud/rest/datastore/datastore.py

+        if not self.session:
+            self.session = requests.Session()
+        session = session or self.session
+        resp = session.post(url, data=payload,


All of these callouts will need to be wrapped in a lock, as with the other gcloud-rest-* projects. Similarly, we'll need to add that argument to Datastore.__init__()

TheKevJames · 2019-06-25T20:19:32Z

datastore/gcloud/rest/datastore/key.py

+
+
+class PathElement(object):
+    def __init__(self, kind, **kwargs):


Could we make id_ and name explicit like we have in gcloud-aio rather than using kwargs?

The thing is, we can't make them keyword-only. And only one of them can be passed here.

One alternative is to force the caller to pass None, "name" when they want to use a name.
Another is to pass an id_or_name and look at the data type.

I'll think about it more.

I think I'm missing something -- why can we not make this def __init__(self, kind, id_=None, name=None): in the same way as we do in aio? I believe aio has a check in the body of the init method (if id_ and name: raise ...)

Nah I'm the one missing something :)
Some habits are hard to break and it didn't occur to me that I could use the method signature you propose and still call it with PathElement(Kind, name='name') - I blame my C# days

TheKevJames · 2019-06-25T20:23:01Z

datastore/gcloud/rest/datastore/query.py

+    # TODO: support `projection` and `distinctOn`
+    def __init__(self,
+                 kind='',            # type: str
+                 query_filter=None,  # type: Filter


This should be Optional[Filter] -- could you go through the type hints and make sure you've marked down which are nullable?

I'll admit I mindlessly converted the Python 3 annotations - gcloud-aio is wrong as well. Will fix

TheKevJames · 2019-06-25T20:25:18Z

gcloud/rest/auth/__init__.py

@@ -1,7 +1,10 @@
 from pkg_resources import get_distribution


Sure thing -- we'll need to make sure these get removed before merge! We'll get the base auth PR in first.

TheKevJames · 2019-07-02T00:03:44Z

This should now be rebase-able onto v1.6.0 :)

KevinGDialpad

A bit of a long review but again, the code comes from gcloud-aio. For the most part, I just converted the type annotations to be Python2-compatible.

I also added the datastore as a separate project, as we do in gcloud-aio. Hoping to deprecate gcloud-rest and break it apart in the future. It just occurred to me that I should update the README and CONTRIBUTING files to explain that.

.pre-commit-config.yaml

KevinGDialpad · 2019-07-03T19:48:46Z

datastore/tests/integration/query_test.py

@@ -0,0 +1,69 @@
+import requests


I split smoke_test.py, hoping to make it easier to add other query-related tests

datastore/tests/unit/value_test.py

…rately

.circleci/config.yml

TheKevJames

Verified tests locally

KevinGDialpad requested review from TheKevJames and a team as code owners June 25, 2019 16:40

ghost requested a review from eddiedialpad June 25, 2019 16:40

KevinGDialpad commented Jun 25, 2019

View reviewed changes

TheKevJames suggested changes Jun 25, 2019

View reviewed changes

feat(datastore): add datastore

d8916f0

KevinGDialpad force-pushed the add_datastore branch from 666d9db to d8916f0 Compare July 3, 2019 19:35

KevinGDialpad changed the title ~~[WIP] feat(datastore): add datastore~~ feat(datastore): add datastore Jul 3, 2019

KevinGDialpad commented Jul 3, 2019

View reviewed changes

Update README and CONTRIBUTING with info about releasing modules sepa…

1c28069

…rately

KevinGDialpad commented Jul 3, 2019

View reviewed changes

.circleci/config.yml Show resolved Hide resolved

Release either gcloud-rest or a single project, depending on tag

b500767

KevinGDialpad commented Jul 4, 2019

View reviewed changes

.circleci/config.yml Show resolved Hide resolved

TheKevJames approved these changes Jul 9, 2019

View reviewed changes

TheKevJames merged commit c6be653 into talkiq:master Jul 9, 2019

KevinGDialpad deleted the add_datastore branch July 9, 2019 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datastore): add datastore #35

feat(datastore): add datastore #35

KevinGDialpad commented Jun 25, 2019

KevinGDialpad left a comment

KevinGDialpad Jun 25, 2019

TheKevJames Jun 25, 2019

KevinGDialpad Jun 25, 2019

KevinGDialpad Jun 26, 2019

TheKevJames Jun 27, 2019

KevinGDialpad Jun 25, 2019

TheKevJames Jun 25, 2019

KevinGDialpad Jun 25, 2019

TheKevJames left a comment

TheKevJames Jun 25, 2019

TheKevJames Jun 25, 2019

TheKevJames Jun 25, 2019

TheKevJames Jun 25, 2019

TheKevJames Jun 25, 2019

KevinGDialpad Jun 25, 2019

TheKevJames Jun 26, 2019

KevinGDialpad Jun 26, 2019

TheKevJames Jun 25, 2019

KevinGDialpad Jun 25, 2019

TheKevJames Jun 25, 2019

TheKevJames commented Jul 2, 2019

KevinGDialpad left a comment

KevinGDialpad Jul 3, 2019

TheKevJames left a comment



		class PathElement(object):
		def __init__(self, kind, **kwargs):

feat(datastore): add datastore #35

feat(datastore): add datastore #35

Conversation

KevinGDialpad commented Jun 25, 2019

KevinGDialpad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheKevJames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheKevJames commented Jul 2, 2019

KevinGDialpad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheKevJames left a comment

Choose a reason for hiding this comment