Skip to content
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: add default values parameter to to_json #164

Merged
merged 2 commits into from
Nov 20, 2020

Conversation

yon-mg
Copy link
Contributor

@yon-mg yon-mg commented Nov 20, 2020

No description provided.

@yon-mg yon-mg requested a review from a team as a code owner November 20, 2020 22:09
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 20, 2020
@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #164 (d20b585) into master (fef7983) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #164   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines          862       862           
  Branches       149       149           
=========================================
  Hits           862       862           
Impacted Files Coverage Δ
proto/message.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fef7983...d20b585. Read the comment docs.

Copy link
Contributor

@software-dov software-dov left a comment

Choose a reason for hiding this comment

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

LGTM.


json2 = Squid.to_json(s).replace(" ", "").replace("\n", "")
assert (
json2 == '{"name":"Steve","massKg":0}' or json2 == '{"massKg":0,"name":"Steve"}'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the ordering not stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No but it shouldn't matter for json objects right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it should matter, I just was curious if you were running into problems during testing.

@software-dov software-dov added the automerge Merge the pull request once unit tests and other checks pass. label Nov 20, 2020
@yon-mg yon-mg merged commit 691f1b2 into googleapis:master Nov 20, 2020
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 20, 2020
instance,
*,
use_integers_for_enums=True,
including_default_value_fields=True
Copy link
Contributor

Choose a reason for hiding this comment

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

@yon-mg Could you open a follow up PR to update the docstring below?

This will result in a change in behavior for someone already using this method Protobuf defaults to False for this option. https://googleapis.dev/python/protobuf/latest/google/protobuf/json_format.html#google.protobuf.json_format.MessageToJson

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand. I did not change the way google.protobuf.json_format.MessageToJson works. In fact this method message.MessageMeta.to_json should call MessageToJson in the same way as it did before if no changes are made to the additional parameter I added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I did neglect to update the docstring for this method which I will update shortly if that's what you mean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants