Skip to content
This repository was archived by the owner on May 7, 2026. It is now read-only.

feat!: Leverage new generator, proto-plus, for google-cloud-datastore - #104

Merged
crwilcox merged 37 commits into
googleapis:masterfrom
crwilcox:new-generator
Oct 30, 2020
Merged

feat!: Leverage new generator, proto-plus, for google-cloud-datastore#104
crwilcox merged 37 commits into
googleapis:masterfrom
crwilcox:new-generator

Conversation

@crwilcox

@crwilcox crwilcox commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

This uses the new microgenerator as the underlying transport for the cloud datastore client

files in services/, as well as tests/gapic, are gen'd

Major Changes: Discontinues python 2.7 support.

release-as: 2.0.0-dev1

@crwilcox crwilcox added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 14, 2020
@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 14, 2020
@crwilcox crwilcox changed the title feat!: Leverage new generator, proto-plus, for google-cloud-datastore. feat!: Leverage new generator, proto-plus, for google-cloud-datastore Oct 14, 2020
@crwilcox

Copy link
Copy Markdown
Contributor Author

PR for samples changes, mostly in admin as it relied on gen surface. GoogleCloudPlatform/python-docs-samples#4858

@crwilcox
crwilcox requested a review from tseaver October 16, 2020 04:31
Comment thread .coveragerc
# See the License for the specific language governing permissions and
# limitations under the License.

# Generated by synthtool. DO NOT EDIT!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really a comment for the generator and not this particular PR, but in general, I'm in favor of clearly marking files which are generated. (I'm assuming this file is still generated.) The less detective work you can make a developer do, the better.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goes for everything under services and types, too. Or maybe it could be a README in those folders. But a clear delineation between generated and hand written code, I think, is helpful.

"""
return entity_pb.properties.get_or_create(name)
properties = entity_pb.properties
try:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later in the same file, you do the same thing at line 396 but in a different way. Maybe a function to standardize the spelling here would be in order.

def _unwrap(pb):
    return getattr(pb, "_pb", pb)

@chrisrossi

Copy link
Copy Markdown
Contributor

Insofar as the only changes to handwritten code seem to be spelling changes to adapt to differences in the underlying generated code, this looks pretty good. Making which bits are generated glaringly obvious, I think, would be good. I don't see anything particularly alarming.

The frequent use of ._pb unwrapping, though, does seem like a code smell. Granted, I'm really only looking at the diff here, so I'm maybe not seeing all the places where we don't need to unwrap the protos, but it does raise the question of why are we wrapping them if we're just going to unwrap them? The couple of spots where we don't know whether we have a wrapped or unwrapped proto are particularly smelly. Can the design to be tweaked so that consumers of protos are getting whichever version, wrapped or unwrapped, that they need?

I realize this is probably a generator question and not a Datastore question.

@crwilcox

Copy link
Copy Markdown
Contributor Author

@chrisrossi Thank you very much for the review. I 100% agree with your feedback. I focused on not breaking any of the system tests and at times this has meant some sort of gross wrapping/unwrapping. I think there is opportunity to clean this up.

@crwilcox
crwilcox merged commit 1723a26 into googleapis:master Oct 30, 2020
@release-please release-please Bot mentioned this pull request Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants