| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Hello @parthea is there any scheduled date for release this pull request? |
Sorry, something went wrong.
|
@parthea Is there anything blocking this PR still? |
Sorry, something went wrong.
There was a problem hiding this comment.
A couple of suggestions, and some questions, but nothing major. This looks good!
Sorry, something went wrong.
| # For backwards compatibility with protobuf 3.x 4.x | ||
| # Remove once support for protobuf 3.x and 4.x is dropped | ||
| # https://github.com/googleapis/python-api-core/issues/643 | ||
| if PROTOBUF_VERSION[0:2] in ["3.", "4."]: | ||
| request_kwargs = json_format.MessageToDict( | ||
| request, | ||
| preserving_proto_field_name=True, | ||
| including_default_value_fields=True, # type: ignore # backward compatibility | ||
| ) | ||
| else: | ||
| request_kwargs = json_format.MessageToDict( | ||
| request, | ||
| preserving_proto_field_name=True, | ||
| always_print_fields_with_no_presence=True, | ||
| ) | ||
|
|
||
| transcoded_request = path_template.transcode(http_options, **request_kwargs) |
There was a problem hiding this comment.
You have this block repeated in four places. I suggest factoring it out into a helper function that only needs to take one parameter:
| # For backwards compatibility with protobuf 3.x 4.x | |
| # Remove once support for protobuf 3.x and 4.x is dropped | |
| # https://github.com/googleapis/python-api-core/issues/643 | |
| if PROTOBUF_VERSION[0:2] in ["3.", "4."]: | |
| request_kwargs = json_format.MessageToDict( | |
| request, | |
| preserving_proto_field_name=True, | |
| including_default_value_fields=True, # type: ignore # backward compatibility | |
| ) | |
| else: | |
| request_kwargs = json_format.MessageToDict( | |
| request, | |
| preserving_proto_field_name=True, | |
| always_print_fields_with_no_presence=True, | |
| ) | |
| transcoded_request = path_template.transcode(http_options, **request_kwargs) | |
| transcoded_request = _transcode_request(request) |
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in 5a49a53
Sorry, something went wrong.
|
|
||
| if prerelease: | ||
| install_prerelease_dependencies( | ||
| session, f"{constraints_dir}/constraints-{PYTHON_VERSIONS[0]}.txt" |
There was a problem hiding this comment.
I'm confused: in line 206, where we call default(..., prerelease=True), the decorator constrains python=PYTHON_VERSIONS[-1]. But here we are referencing the constraints for PYTHON_VERSIONS[0]. Could you clarify?
Sorry, something went wrong.
There was a problem hiding this comment.
As per the comment in below, the constraints file for the lowest supported python version contains a list of all of the dependencies of the library. The function install_prerelease_dependencies will extract the dependencies from the file.
python-api-core/testing/constraints-3.7.txt
Lines 1 to 7 in 126b5c7
Lines 79 to 88 in 7fbce0d
Once we have a list of dependencies, we will install them independently with the --pre and --no-deps option. `--upgrade is also added to ensure that we get the latest pre-release.
Lines 99 to 100 in 7fbce0d
Sorry, something went wrong.
|
|
||
|
|
||
| def default(session, install_grpc=True): | ||
| def install_prerelease_dependencies(session, constraints_path): |
There was a problem hiding this comment.
Sorry, I have gaps in my mental model for testing prerelease;
This installs our dependencies at pre-release versions, right?
Is the only difference, then, the use of --pre below? If it's the same set of dependencies, it seems it would be clearer to have them loaded in the same place for pre-release and stable versions, and add the extra install parameters conditionally on each thing we install.
(It might be easier to talk synchronously about this.)
Sorry, something went wrong.
There was a problem hiding this comment.
There is a difference in that we don't allow transitive dependencies in the install_prerelease_dependencies session as we want to ensure that we're installing the pre-release version of each dependency. In the non-prerelease session, we are installing transitive dependencies.
Sorry, something went wrong.
|
|
||
| @nox.session(python=["3.7", "3.8", "3.9", "3.10", "3.11", "3.12"]) | ||
| @nox.session(python=PYTHON_VERSIONS[-1]) | ||
| def unit_prerelease(session): |
There was a problem hiding this comment.
So this tests running against the pre-release versions of the dependencies, right? And we only bother to do this with the latest Python run-time.
A comment might be helpful. Also, my personal preference would to rename this (and similar code elsewhere) as unit_with_prerelease_deps. Every time I read something like unit_prerelease my mind first assumes we're testing the pre-release version of this library, not of its dependencies.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in e1efa83
Sorry, something went wrong.
| strategy: | ||
| matrix: | ||
| option: ["", "_grpc_gcp", "_wo_grpc"] | ||
| option: ["", "_grpc_gcp", "_wo_grpc", "_prerelease"] |
There was a problem hiding this comment.
I think this more succinct code should work (I haven't tested it) and would be easier to maintain.
Ref: https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#expanding-or-adding-matrix-configurations
option: ["", "_grpc_gcp", "_wo_grpc"]
python:
- "3.7"
- "3.8"
- "3.9"
- "3.10"
- "3.11"
- "3.12"
exclude:
- option: "_wo_grpc"
python: 3.7
- option: "_wo_grpc"
python: 3.8
- option: "_wo_grpc"
python: 3.9
include:
- option: "_prerelease"
python: 3.12
Sorry, something went wrong.
There was a problem hiding this comment.
This comment is obsolete with the changes in e1efa83
Sorry, something went wrong.
…gleapis/python-api-core into add-support-for-protobuf-5-x
| from google.api_core.operations_v1.abstract_operations_client import ( | ||
| AbstractOperationsClient, | ||
| ) |
There was a problem hiding this comment.
Why is this file changed?
Sorry, something went wrong.
There was a problem hiding this comment.
I ran black on all of the files and this file was formatted
Sorry, something went wrong.
|
|
||
|
|
||
| @nox.session(python="3.8") | ||
| @nox.session(python=DEFAULT_PYTHON_VERSION) |
There was a problem hiding this comment.
Just to clarify, do we want to run this against all versions?
Sorry, something went wrong.
There was a problem hiding this comment.
We normally only run it on 1 version but we can expand it if there is value
Sorry, something went wrong.
| including_default_value_fields=False, | ||
| preserving_proto_field_name=False, | ||
| use_integers_for_enums=False, | ||
| ) |
There was a problem hiding this comment.
This is essentially also converting a message to a dict. So we could use a similar approach to what we're doing for request_params and define a helper function so we're consistent. But it's more of a preference and could be a follow up.
I'll leave it up to you since this isn't a blocker!
Sorry, something went wrong.
There was a problem hiding this comment.
I took a closer look at making this change and I don't believe the helper function will result in fewer lines of code. The reason that we had the helper function elsewhere was to remove duplication for the protobuf 3.x/4.x compatibility code but we don't have the duplication here.
Sorry, something went wrong.
|
Hello, I am eagerly anticipating the next release! Could you please provide any updates on the release date? Thank the whole team for your hard work and dedication. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Fixes #628 🦕
Fixes #641 🦕
Fixes #642 🦕
Regarding the including_default_value_fields argument of json_format.MessageToDict() in protobuf:
In protobuf version 3.19.6 - The default value of including_default_value_fields was False
https://github.com/protocolbuffers/protobuf/blob/5cba162a5d93f8df786d828621019e03e50edb4f/python/google/protobuf/json_format.py#L92
At the time that the argument was removed in protobuf 5.x - the default value of including_default_value_fields was False
protocolbuffers/protobuf@2699579#diff-8de817c14d6a087981503c9aea38730b1b3e98f4e306db5ff9d525c7c304f234
IOW, setting argument including_default_value_fields to False had no effect as it was the default behaviour.
The Unit tests / unit_prerelease-3.12 presubmit check was added to run tests against the latest pre-release version of protobuf as several dependencies have a constraint on protobuf<5