New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: Leverage new generator, proto-plus, for google-cloud-datastore #104
Conversation
2812ce9
to
da8300d
Compare
PR for samples changes, mostly in admin as it relied on gen surface. GoogleCloudPlatform/python-docs-samples#4858 |
@@ -14,25 +14,23 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
# Generated by synthtool. DO NOT EDIT! |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -86,7 +86,14 @@ def _new_value_pb(entity_pb, name): | |||
:rtype: :class:`.entity_pb2.Value` | |||
:returns: The new ``Value`` protobuf that was added to the entity. | |||
""" | |||
return entity_pb.properties.get_or_create(name) | |||
properties = entity_pb.properties | |||
try: |
There was a problem hiding this comment.
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)
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 I realize this is probably a generator question and not a Datastore question. |
@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. |
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