search

bivas / protobuf-java-format

Provide serialization and de-serialization of different formats based on Google’s protobuf Message. Enables overriding the default (byte array) output to text based formats such as XML, JSON and HTML.
BSD 3-Clause "New" or "Revised" License
153 stars 97 forks source link

Correctly slurp unknown fields into message that are unknown at both serialization and deserialization time #26

Closed primshnick closed 7 years ago

primshnick commented 8 years ago

For some reason, in the old PR, Github wasn't recognizing the force push I made to reduce the PR to one commit. So opening a new PR.

As specified in the title, this PR allows for unknown fields to be correctly handled in the json to proto conversion. Previously, only fields that were unknown at the time of json serialization (but known at deserialization time) were handled, and any others were skipped over.

scr commented 8 years ago

Can you add a test for JsonJacksonFormat too?

scr commented 8 years ago

Out of curiosity, what was the "old PR"? Can you paste a link to the one this is replacing in a comment?

primshnick commented 8 years ago

Sorry, #25 was the "old PR".

primshnick commented 8 years ago

Hi Sheridan,

Thanks for the comments! I probably won't have a chance to respond until this weekend though..

primshnick commented 8 years ago

Just an update here. I believe I'm very close to finishing a new commit that has 1) the ability to handle the other data types besides lengthDelimited and 2) very thorough testing. However, I'm being blocked by issue #28. I can try to look into this new issue as well, but not sure exactly when I'll get a chance.

primshnick commented 8 years ago

Ok, my latest commit: 1) Fixes the escape/unescape bug (issue #28), 2) Adds (disabled) tests of the same issue for JsonJacksonFormat, 3) Adds support for wire types other than lengthDelimited, 4) Addresses other minor comments by Sheridan.

I haven't used the JsonJackson stuff before so please double check that I did the right thing in those tests. Thanks!

scr commented 8 years ago

When commenting in github, please put a '#' character before numbers that refer to issues or PR's to make them link/references to each other.

primshnick commented 8 years ago

Sure, will do in future

scr commented 8 years ago

@PeteyPabPro - the reason GitHub isn't behaving (If I understand correctly) is that you are making a pull request from your fork's master.

You should always make PR's from a branch on your fork - this is because the expected github workflow is to create the PR, then to merge it, and finally to delete the branch - you cannot (easily) delete your HEAD branch (which by default is master) and this causes confusion with github if you've ever used that branch for another PR - github really liked "new" branches for each PR.

So… in the future, so something like this when starting work for a PR:

First, you should have two remotes in your local clone - do this once, or read git help remote for more info on how to get your set up correct

git remote origin set-url git@github.com:bivas/protobuf-java-format.git # point origin at source of truth
git remote origin set-head origin master # set the HEAD branch - just to reduce typing: git merge origin will do git merge origin/master :D
git remote add PeteyPabPro git@github.com:PeteyPabPro/protobuf-java-format.git # point the remote named PeteyPabPro to your fork
git fetch --all # fetch all remotes to your .git (doesn't check anything out or alter your working dir).

Then, you can set up a new branch and push it to your fork with upstream-tracking.

git fetch origin
git checkout -b my-workbranch-name origin # or origin/master if you didn't set-head.
git push -u PeteyPabPro HEAD # HEAD is a convenience to push your currently checked-out branch.

Once you've made changes on that branch and pushed again, you should see the PR-creation notification as you did before to merge your new branch to bivas' master.

scr commented 8 years ago

If you can, I'd like to split this into separate concerns/issues - I realize they're somewhat interdependent, but you can commit each fix with its own unit test and have smaller patches that have more focused changes. Ping me on G+ or hangouts when you have something to review as I don't check the email that github sends terribly regularly. You can keep this patch around and when the others land they should merge into this one cleanly (I hope).

primshnick commented 8 years ago

Ok, will see what I can do as far as splitting it up, and will also consider your new comments/questions...