Resolve #10949: use raise from in json_format.py #10966
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
I noticed we don't have python unit tests. Why's that? Or I missed sth. |
|
You might be looking for https://github.com/protocolbuffers/protobuf/blob/main/python/google/protobuf/internal/json_format_test.py? In general you can find python unit tests: Line 158 in 2880fef |
| raise TypeError( | ||
| 'Can not find message descriptor by type_url: {0}'.format(type_url)) | ||
| 'Can not find message descriptor by type_url: {0}'.format(type_url)) from e |
There was a problem hiding this comment.
This line is more than 80 columns wide; please shrink it. I suggest:
- raise TypeError('Can not find message descriptor by type_url: {0}'.format(
-
type_url)) from e
| @@ -626,28 +626,28 @@ def _ConvertFieldValuePair(self, js, message, path): | |||
| '{0}.{1}'.format(path, name))) | |||
| except ParseError as e: | |||
| if field and field.containing_oneof is None: | |||
| raise ParseError('Failed to parse {0} field: {1}.'.format(name, e)) | |||
| raise ParseError('Failed to parse {0} field: {1}.'.format(name, e)) from e | |||
There was a problem hiding this comment.
This line is more than 80 columns wide; please shrink it. I suggest a line-break after the comma:
+ raise ParseError('Failed to parse {0} field: {1}.'.format(name,
+ e)) from e
Same for lines 633 and 635
| raise ParseError( | ||
| '@type is missing when parsing any message at {0}'.format(path)) | ||
| '@type is missing when parsing any message at {0}'.format(path)) from e |
There was a problem hiding this comment.
This line is more than 80 columns wide; please shrink it. I suggest a line-break before path:
+ '@type is missing when parsing any message at {0}'.format(
+ path)) from e
TommyDew42
requested review from
jorgbrown and
zhangskz
and removed request for
anandolee,
jorgbrown and
zhangskz
|
Seems the |
|
There isn't much information in the error page. Would the maintainers have more insight into the CI errors? I thought the feature branch might be missing sth from the main so it caused the CI errors but my merging failed. How should we merge the master into the feature branch? 😢 |
I believe the ruby error was failing on main and should be addressed now. Rerunning Kokoro, but I believe you may need to rebase your PR. |
|
I did a rebase. The And out of curiosity, why is merge not encouraged? |
You can ignore the Mergeable check -- that failure should always be present to remind Protobuf maintainers to not merge PRs directly and let our automated Copybara system handle merging changes in Github. This is because we land our changes internally first. |
|
Do we need one more approval to unblock merging this PR? Tagging @jorgbrown to help!! |
|
Would @acozzette be able to help review? All relevant changes in the PR is in python/google/protobuf/json_format.py!! |
|
Undid the rebase by git reset and force push to make the PR reviewable again |
Resolve #10949