Change the behavior of the fixup script (#291)

Only script parameters are a single input dir and a single output dir
Fixups are no longer in-place: they are written to the corresponding
path in the output dir. The files in the input dir are unmodified.

Fixup script works on all services in an api concurrently instead of
one script per service. It has been renamed accordingly.

Author: software-dov

…es/scripts/fixup_%service_keywords.py.j2 …c/templates/scripts/fixup_keywords.py.j2packages/gapic-generator/gapic/templates/scripts/fixup_%service_keywords.py.j2 renamed to packages/gapic-generator/gapic/templates/scripts/fixup_keywords.py.j2 packages/gapic-generator/gapic/templates/scripts/fixup_%service_keywords.py.j2 renamed to packages/gapic-generator/gapic/templates/scripts/fixup_keywords.py.j2

@@ -3,7 +3,8 @@
 import argparse
 import os
 import libcst as cst
-from itertools import chain
+import pathlib
+import sys
 from typing import (Any, Callable, Dict, List, Sequence, Tuple)
 
 
@@ -21,14 +22,18 @@ def partition(
     return results[1], results[0]
 
 
-class {{ service.client_name }}CallTransformer(cst.CSTTransformer):
+class {{ api.naming.module_name }}CallTransformer(cst.CSTTransformer):
     CTRL_PARAMS: Tuple[str] = ('retry', 'timeout', 'metadata')
-
+    {% with all_methods = [] -%}
+    {% for service in api.services.values() %}{% for method in service.methods.values() -%}
+    {% do all_methods.append(method) -%}
+    {% endfor %}{% endfor -%}
     METHOD_TO_PARAMS: Dict[str, Tuple[str]] = {
-        {% for method in service.methods.values() -%}
+    {% for method in all_methods|sort(attribute='name')|unique(attribute='name') -%}
           '{{ method.name|snake_case }}': ({% for field in method.legacy_flattened_fields.values() %}'{{ field.name }}', {% endfor %}),
-        {% endfor -%}
+    {% endfor -%}
     }
+    {% endwith %}
 
     def leave_Call(self, original: cst.Call, updated: cst.Call) -> cst.CSTNode:
         try:
@@ -59,11 +64,11 @@ class {{ service.client_name }}CallTransformer(cst.CSTTransformer):
             value=cst.Dict([
                 cst.DictElement(
                     cst.SimpleString("'{}'".format(name)),
-                    {# Inline comments and formatting are currently stripped out. #}
-                    {# My current attempts at preverving comments and formatting #}
-                    {# keep the comments, but the formatting is run through a log #}
-                    {# chipper, and an extra comma gets added, which causes a #}
-                    {# parse error. #}
+                    {# Inline comments and formatting are currently stripped out. -#}
+                    {# My current attempts at preverving comments and formatting -#}
+                    {# keep the comments, but the formatting is run through a log -#}
+                    {# chipper, and an extra comma gets added, which causes a -#}
+                    {# parse error. -#}
                     cst.Element(value=arg.value)
                 )
                 # Note: the args + kwargs looks silly, but keep in mind that
@@ -79,31 +84,45 @@ class {{ service.client_name }}CallTransformer(cst.CSTTransformer):
 
 
 def fix_files(
-    dirs: Sequence[str],
-    files: Sequence[str],
+    in_dir: pathlib.Path,
+    out_dir: pathlib.Path,
     *,
-    transformer={{ service.client_name }}CallTransformer(),
+    transformer={{ api.naming.module_name }}CallTransformer(),
 ):
-    pyfile_gen = chain(
-        (os.path.join(root, f)
-         for d in dirs
-         for root, _, files in os.walk(d)
-         for f in files if os.path.splitext(f)[1] == ".py"),
-        files)
+    """Duplicate the input dir to the output dir, fixing file method calls.
+
+    Preconditions:
+    * in_dir is a real directory
+    * out_dir is a real, empty directory
+    """
+    pyfile_gen = (
+        pathlib.Path(os.path.join(root, f))
+        for root, _, files in os.walk(in_dir)
+        for f in files if os.path.splitext(f)[1] == ".py"
+    )
 
     for fpath in pyfile_gen:
-        with open(fpath, 'r+') as f:
+        with open(fpath, 'r') as f:
             src = f.read()
-            tree = cst.parse_module(src)
-            updated = tree.visit(transformer)
-            f.seek(0)
-            f.truncate()
+
+        # Parse the code and insert method call fixes.
+        tree = cst.parse_module(src)
+        updated = tree.visit(transformer)
+
+        # Create the path and directory structure for the new file.
+        updated_path = out_dir.joinpath(fpath.relative_to(in_dir))
+        updated_path.parent.mkdir(parents=True, exist_ok=True)
+
+        # Generate the updated source file at the corresponding path.
+        with open(updated_path, 'w') as f:
             f.write(updated.code)
 
 
 if __name__ == '__main__':
     parser = argparse.ArgumentParser(
-        description="""Fix up source that uses the {{ service.name }} client library.
+        description="""Fix up source that uses the {{ api.naming.module_name }} client library.
+
+The existing sources are NOT overwritten but are copied to output_dir with changes made.
 
 Note: This tool operates at a best-effort level at converting positional
       parameters in client method calls to keyword based parameters.
@@ -114,32 +133,40 @@ Note: This tool operates at a best-effort level at converting positional
 
       These all constitute false negatives. The tool will also detect false
       positives when an API method shares a name with another method.
-
-      Be sure to back up your source files before running this tool and to compare the diffs.
 """)
-    group = parser.add_mutually_exclusive_group(required=True)
-    group.add_argument(
+    parser.add_argument(
         '-d',
-        metavar='dir',
-        dest='dirs',
-        action='append',
-        help='a directory to walk for python files to fix up',
+        dest='input_dir',
+        help='the input directory to walk for python files to fix up',
     )
-    group.add_argument(
-        '-f',
-        metavar='file',
-        dest='files',
-        action='append',
+    parser.add_argument(
+        '-o',
+        dest='output_dir',
         help='a file to fix up via un-flattening',
     )
     args = parser.parse_args()
-    print(
-        """It is strongly, strongly recommended that you commit outstanding changes and
-back up your source tree before running this conversion script.
-Please take a moment to do that now if you haven't already.
-"""
-    )
-    resp = input("Really attempt to convert sources? yes/[no]: ")
-    if resp == "yes":
-        fix_files(args.dirs or [], args.files or [])
+    input_dir = pathlib.Path(args.input_dir)
+    output_dir = pathlib.Path(args.output_dir)
+    if not input_dir.is_dir():
+        print(
+            f"input directory '{input_dir}' does not exist or is not a directory",
+            file=sys.stderr,
+        )
+        sys.exit(-1)
+
+    if not output_dir.is_dir():
+        print(
+            f"output directory '{output_dir}' does not exist or is not a directory",
+            file=sys.stderr,
+        )
+        sys.exit(-1)
+
+    if os.listdir(output_dir):
+        print(
+            f"output directory '{output_dir}' is not empty",
+            file=sys.stderr,
+        )
+        sys.exit(-1)
+
+    fix_files(input_dir, output_dir)
 {% endblock %}

packages/gapic-generator/gapic/templates/setup.py.j2

@@ -25,11 +25,7 @@ setuptools.setup(
         'libcst >= 0.2.5',
     ],
     scripts=[
-        {% for proto in api.all_protos.values() -%}
-        {% for service in proto.services.values() -%}
-        'scripts/fixup_{{ service.module_name }}_keywords.py',
-        {% endfor -%}
-        {% endfor -%}
+        'scripts/fixup_keywords.py',
     ],
     classifiers=[
         'Development Status :: 3 - Alpha',