fixup_pubsub_v1_keywords.py generates incorrect output in some cases

[rvandegrift]

I just tried the fixup script and ran into a few issues.

1. It generates this change even though PublisherClient.publish does not take the new style parameters:

-        publisher_client.publish(topic, bytes(json.dumps(message), 'utf-8'))
+        publisher_client.publish(request = {'topic': topic, 'messages': bytes(json.dumps(message), 'utf-8')})

2. It generates this change even though request and keyword args are documented to be mutually exclusive:

-        response = subscriber.pull(subscription_path, max_messages=NUM_MESSAGES, timeout=TIMEOUT)
+        response = subscriber.pull(
+            request = {'subscription': subscription_path, 'max_messages': NUM_MESSAGES}, timeout=TIMEOUT,
+        )

3. all of the generated request keyword arguments break pep8 (they should not have spaces around =)

Stylistically, I find the keyword arguments better to read than all of the inline dicts for the request syntax. It'd be great if the tool had an option to emit the keyword args version.

[plamut]

@rvandegrift The script indeed produces several false positives, unfortunately, since not all method signatures have been changed (as mentioned in the UPGRADING guide).

@cguardia Sounds like a pretty self-contained task, would you like to take a shot at it?

[cguardia]

@plamut the script does need a couple of fixes, including the style guide improvements suggested in the comments on #203. Maybe instead of opening a new issue for that I can take this chance to fix both things?

[plamut]

@cguardia Since these two are related and probably not-to-large, it's fine to fix them in one go, yes.

[cguardia]

@plamut there's something off here. The script specifically separates retry, timeout, and metadata, and sends them always as keyword arguments. This means the request parameter will always be combined with keyword arguments for these values when using the script. However, as noted by @rvandegrift, the documentation states that combing e.g. metadata and request as keyword arguments is invalid and will cause an error. Obviously, either the script or the documentation are wrong, but it's not a bug, it's a design decision. The script can easily be changed, but do you know why it does this?

[plamut]

@cguardia I don't know, but if I had to guess, I think the script was meant to be used on the 1.x code to make the upgrading somewhat less tedious and thus doesn't expect the request parameter to be present at all.

The documentation is correct, however, as using both the request and kwargs on a method results in an error, thus it's the script that should probably be changed.

[cguardia]

@plamut it sets the request parameter, so it knew about it, but since the docs are correct, I'll change the script accordingly. Thanks.

[plamut]

Ah, it does? Then it must be a bug in the script, yes.