Creating `aspect` option (ALL, CODE, PACKAGE) by garrettjonesgoogle · Pull Request #475 · googleapis/artman

garrettjonesgoogle · 2018-07-25T15:58:39Z

This will only work for C# so far; after googleapis/gapic-generator#2166 , it will work for Java too.

vam-google

In general looks good, with several comments.

artman/pipelines/grpc_generation.py

        return task_utils.instantiate_tasks(tasks, kwargs)

    def get_grpc_codegen_tasks(self, **kwargs):
+        if self.language == 'java':


artman/cli/main.py

        help='[Required] Name of the artifact for artman to generate. Must '
        'match an artifact in the artman config yaml.')
+    parser_generate.add_argument(
+        '--aspect',


artman/tasks/gapic_tasks.py

        args = args + gapic_args
+
+        gapic_artifact = ''
+        if aspect == 'ALL':


artman/config/loader.py



-def load_artifact_config(artman_config_path, artifact_name):
+def load_artifact_config(artman_config_path, artifact_name, aspect=None):


vam-google

LGTM, but I still don't like the huge get_grpc_codegen_tasks method.

artman/cli/main.py

        help='[Required] Name of the artifact for artman to generate. Must '
        'match an artifact in the artman config yaml.')
+    parser_generate.add_argument(
+        '--aspect',


artman/pipelines/grpc_generation.py

        return task_utils.instantiate_tasks(tasks, kwargs)

    def get_grpc_codegen_tasks(self, **kwargs):
+        if self.language == 'java':


garrettjonesgoogle · 2018-07-25T23:10:59Z

ALL vs CODE_AND_PACKAGE: I considered the second, but I wanted to allow for adding a third item, and the default should be to include everything I think.

garrettjonesgoogle · 2018-07-25T23:22:57Z

I split apart the long function, PTAL

Creating aspect option (ALL, CODE, PACKAGE)

eb714f7

garrettjonesgoogle requested a review from vam-google July 25, 2018 15:58

garrettjonesgoogle mentioned this pull request Jul 25, 2018

Using --aspect CODE from artman; supporting java_proto googleapis/google-cloud-java#3507

Merged

vam-google reviewed Jul 25, 2018

View reviewed changes

vam-google approved these changes Jul 25, 2018

View reviewed changes

PR feedback

f8fa838

garrettjonesgoogle merged commit 8662dfe into googleapis:master Jul 26, 2018

alexander-fenster mentioned this pull request Jul 26, 2018

smoke tests are failing #479

Closed



		def load_artifact_config(artman_config_path, artifact_name):
		def load_artifact_config(artman_config_path, artifact_name, aspect=None):

Conversation

garrettjonesgoogle commented Jul 25, 2018

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

garrettjonesgoogle commented Jul 25, 2018

Uh oh!

garrettjonesgoogle commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants