Refactor to support the finalized-ish input format. #6

lukesneeringer · 2018-07-12T00:37:29Z

This is a refactor to support the input format we landed on.

It does a lot of things:

Uses the new annotations.
Moves templates to templates/ in the project root.
Removes support for flat files, which are no longer needed with @garrettjonesgoogle's package generation work.
It refactors the API class to maintain some separation between the source proto files. (I want this because I eventually may want some per-proto functionality.)
API and Proto are now frozen dataclasses. In order to support this, there are build class methods on both.
A new Naming class is added, and naming-based things are moved there (from API).
Updates documentation to be current.
Officially supports Python 3.7.

codecov · 2018-07-12T00:41:11Z

Codecov Report

Merging #6 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master     #6   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     14    +1     
  Lines         316    378   +62     
  Branches       44     58   +14     
=====================================
+ Hits          316    378   +62

Impacted Files	Coverage Δ
api_factory/schema/naming.py	`100% <100%> (ø)`
api_factory/schema/api.py	`100% <100%> (ø)`	⬆️
api_factory/generator/generator.py	`100% <100%> (ø)`	⬆️
api_factory/utils/cache.py	`100% <100%> (ø)`	⬆️
api_factory/schema/wrappers.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3ae3e6...39cd08f. Read the comment docs.

pongad

I don't know enough Python to comment on most of this :P

api_factory/cli/generate.py

@@ -40,7 +42,12 @@ def generate(

    # Translate into a protobuf CodeGeneratorResponse;
    # if there are issues, error out appropriately.


landrito

The Naming class seems inconsistent. The wrapper classes have their naming methods included in the class itself without adding a new naming class. Can you provide more context around adding the Naming class?

lukesneeringer · 2018-07-12T19:31:50Z

The wrapper classes generally have "their name" and nothing else. This has a bunch of names and variants.

landrito

There are a lot of frozen dataclasses that are using the @Property annotation rather than the @cached_property annotation. What is your threshold for using @cached_property?

api_factory/schema/api.py

-                the messages for which being laoded. In other words, this is
-                the segment of the source info that has paths matching
-                or within ``children``.
+            path (Tuple[int]): The location path up to this point. This is


api_factory/schema/api.py

-                       address: metadata.Address, info: dict) -> None:
-        """Load arbitrary children from a Descriptor.
+
+    def _load_children(self, children: Sequence, loader: Callable, *,


api_factory/schema/api.py

@@ -258,34 +317,34 @@ def _load_descriptor(self, message_pb: descriptor_pb2.DescriptorProto,
        fields = self._get_fields(
            message_pb.field,
            address=nested_addr,
-            info=info.get(2, {}),
+            path=path + (2,),
        )
        fields.update(self._get_fields(


api_factory/schema/api.py

        self._load_children(message_pb.enum_type, address=nested_addr,
-                            loader=self._load_enum, info=info.get(4, {}))
+                            loader=self._load_enum, path=path + (4,))
        # self._load_children(message.oneof_decl, loader=self._load_field,


api_factory/schema/api.py

+            answer[meth_pb.name] = wrappers.Method(
+                input=self.all_messages[meth_pb.input_type.lstrip('.')],
+                lro_metadata=self.all_messages.get(types.metadata, None),
+                lro_payload=self.all_messages.get(types.response, None),


docs/api-configuration.rst

-This is rolled up into a strcuture spelled ``Client``, and the annotation
-is defined in `google/api/experimental/client.proto`_.
+This is rolled up into a strcuture spelled ``Metadata``, and the annotation
+is defined in `google/api/metadata.proto`_.


lukesneeringer · 2018-07-12T23:08:02Z

There are a lot of frozen dataclasses that are using the @Property annotation rather than the @cached_property annotation. What is your threshold for using @cached_property?

I use @cached_property if something non-trivial is being computed, or in cases where we return instances and it matters that the instances be the same object rather than an equivalent copy.

api_factory/schema/naming.py

+        # This code may look counter-intuitive (why not use ? to make it
+        # optional), but the engine's greediness routine will decide that
+        # the version is the name, which is not what we want.
+        version = r'\.(?P<version>v[0-9]+(p[0-9]+)?((alpha|beta|test)[0-9])*)'


api_factory/schema/wrappers.py

+        """Return the field headers defined for this method."""
+        http = self.options.Extensions[annotations_pb2.http]
+        if http.get:
+            return tuple(re.findall(r'\{([a-z][\w\d_.]+)=', http.get))


api_factory/templates/$namespace/$name_$version/$service/transports/http.py.j2

@@ -88,7 +88,12 @@ class {{ service.name }}HttpTransport({{ service.name }}Transport):

        # Send the request.
        response = self._session.post(
-            f'https://{self.SERVICE_ADDRESS}/$rpc/{{ '.'.join(method.meta.address.package) }}.{{ service.name }}/{{ method.name }}',
+            'https://{host}/$rpc/{package}.{service}/{method}'.format(


docs/api-configuration.rst

+library will work.
+
+Look for values enclosed by ``<<<`` and ``>>>`` to quickly spot these.
+As of this writing, these are the ``SERVICE_ADDRESS`` and ``OAUTH_SCOPES``


tests/unit/schema/test_api.py

+        assert args[1] == pb._load_service
+
+
+def test_not_taget_file():


landrito

LGTM after the following issues are filed:

regex on the version matcher
LRO silent failure

alexander-fenster · 2019-03-20T20:28:41Z

api_factory/schema/naming.py

+        #
+        # This code may look counter-intuitive (why not use ? to make it
+        # optional), but the engine's greediness routine will decide that
+        # the version is the name, which is not what we want.


Note: the engine's greediness can be limited by appending ? to + or *. If the namespace pattern is changed to (?P<namespace>[a-z0-9_.]+?) it will never eat more than it's supposed to.

Luke Sneeringer added 15 commits July 6, 2018 18:56

WIP

242a62c

WIP

aecdf34

wip

f2dad3a

wip

8eb2b54

Now getting through all Python code, to Jinja code.

5420527

Able to write a (not working) library.

ae31590

Fix hostname.

dd7af5c

The client works again!

470d5ec

Remove Pipfile.

25838d6

Fixing tests, adding Python 3.7

a2d75f5

One more working test.

ea2bb39

Full testing for naming class.

350a598

WIP

ea5029b

The tests work

646be86

Update documentation.

495ffff

lukesneeringer self-assigned this Jul 12, 2018

lukesneeringer requested review from pongad, eoogbe and landrito July 12, 2018 00:37

Fix CircleCI config to run unit tests on both 3.6 and 3.7.

0b7d4bd

pongad reviewed Jul 12, 2018

View reviewed changes

api_factory/cli/generate.py Outdated

@@ -40,7 +42,12 @@ def generate(

# Translate into a protobuf CodeGeneratorResponse;

# if there are issues, error out appropriately.

This comment was marked as spam.

Sign in to view

Luke Sneeringer added 2 commits July 12, 2018 06:18

Update comments to reflect @pongad feedback.

fcb6e15

Add naming module to docs.

3b59805

landrito reviewed Jul 12, 2018

View reviewed changes

Luke Sneeringer added 2 commits July 12, 2018 16:10

Correct comment.

02dacc6

Address @landrito doc comment.

735cdf2

landrito reviewed Jul 13, 2018

View reviewed changes

Address @landrito feedback.

a4ce202

lukesneeringer mentioned this pull request Jul 13, 2018

HTTP/1 transport should return callables. #7

Closed

More feedback.

39cd08f

landrito approved these changes Jul 13, 2018

View reviewed changes

lukesneeringer merged commit 71845cd into master Jul 13, 2018

lukesneeringer deleted the input-refactor branch July 13, 2018 18:05

alexander-fenster reviewed Mar 20, 2019

View reviewed changes

		@@ -40,7 +42,12 @@ def generate(

		# Translate into a protobuf CodeGeneratorResponse;
		# if there are issues, error out appropriately.

		assert args[1] == pb._load_service


		def test_not_taget_file():

Refactor to support the finalized-ish input format. #6

Refactor to support the finalized-ish input format. #6

Conversation

lukesneeringer commented Jul 12, 2018 • edited Loading

codecov bot commented Jul 12, 2018 • edited Loading

Codecov Report

pongad left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

landrito left a comment

Choose a reason for hiding this comment

lukesneeringer commented Jul 12, 2018

landrito left a comment

Choose a reason for hiding this comment

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.

This comment was marked as spam.

lukesneeringer commented Jul 12, 2018

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.

landrito left a comment

Choose a reason for hiding this comment

alexander-fenster Mar 20, 2019

Choose a reason for hiding this comment

lukesneeringer commented Jul 12, 2018 •

edited

Loading

codecov bot commented Jul 12, 2018 •

edited

Loading