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

jsonpb: improve error reporting #310

Closed
wants to merge 1 commit into from

Conversation

mwitkow
Copy link

@mwitkow mwitkow commented Mar 15, 2017

This PR addresses the issue of:
#266

See the added test for the modification in error handling

@dsnet
Copy link
Member

dsnet commented Dec 14, 2017

Adding error diagnostics sounds fine, but there is no reason to export the error type. Please unexport that.

@dsnet dsnet self-requested a review December 14, 2017 19:15
@dsnet dsnet changed the title Add nicejsonpb error handling jsonpb: improve error reporting Mar 9, 2018
@mwitkow mwitkow force-pushed the feature/nicejsonpb branch from 3cc6773 to 0c2a563 Compare March 31, 2018 11:18
@mwitkow
Copy link
Author

mwitkow commented Mar 31, 2018

I've rebased on latest master, and made the error struct private. @dsnet can you PTAL?

CC @io-ben @Helcaraxan

@dsnet
Copy link
Member

dsnet commented Mar 31, 2018

Hi, thanks for working on this. Apologies for the long delay in reviewing. Can you actually rebase against the dev branch?

@mwitkow mwitkow changed the base branch from master to dev April 1, 2018 08:01
@mwitkow mwitkow force-pushed the feature/nicejsonpb branch from 0c2a563 to 42d8fe0 Compare April 1, 2018 08:48
@mwitkow
Copy link
Author

mwitkow commented Apr 1, 2018

@dsnet done, it's now against dev. had to rewrite a bunch of commits.

@dsnet
Copy link
Member

dsnet commented Apr 3, 2018

There are some test failures, fix?

@mwitkow
Copy link
Author

mwitkow commented Apr 3, 2018

😕

--- FAIL: TestUnmarshalReturnsUsefulErrors (0.00s)
	jsonpb_test.go:1210: in UnknownFieldErrors expected error 
		'unparsable field Simple: fields [unknownOne unknownTwo] do not exist in set of known fields [oBool oInt32 oInt64 oUint32 oUint64 oSint32 oSint64 oFloat oDouble oString oBytes]'
		 but got 
		'unparsable field Simple: fields [unknownTwo unknownOne] do not exist in set of known fields [oBool oInt32 oInt64 oUint32 oUint64 oSint32 oSint64 oFloat oDouble oString oBytes]'
FAIL
FAIL	github.com/golang/protobuf/jsonpb	0.028s

and that's on the 1.x run, which is using 1.10, while the 1.10.x test test run worked. Looks like a flake
How do I retrigger this run?

@healiha
Copy link

healiha commented Apr 26, 2018

Would be great if we could finally have this improvement. You could --amend --no-edit your commit and force push to trigger a new build, although I'm not sure that's the best way to do it.

cc @dsnet

@base698
Copy link

base698 commented Jun 22, 2018

Could also revert, then revert the revert and squash it. I really need this feature as well...

@mwitkow mwitkow force-pushed the feature/nicejsonpb branch from 42d8fe0 to 50f00ee Compare June 22, 2018 15:13
@mwitkow
Copy link
Author

mwitkow commented Jun 22, 2018

rebased on top of latest dev and force pushed. Let's hope the Travis gods are benign ;)

kung-foo added a commit to s55-labs/protobuf that referenced this pull request Jul 17, 2018
@spenczar
Copy link
Contributor

I'm interested in reviving this. @mwitkow, what's the status here? I can jump in.

@dsnet
Copy link
Member

dsnet commented Jun 11, 2019

jsonpb has been completely re-written in v2, so all of this is probably moot.

See https://github.com/golang/protobuf/tree/api-v2/encoding/protojson

If the errors produced by v2 are still insufficient, it'd be good to improve the errors there.

@spenczar
Copy link
Contributor

I haven't looked at the v2 errors yet, I'm not sure. I just got here from twitchtv/twirp#148, through #266.

Is there a master issue for v2, or a targeted release date that I can track against?

@dsnet
Copy link
Member

dsnet commented Jun 11, 2019

The closest thing to a "master" issue is #364, which was the primary reason for a v2.

There is no official release date, but we've been aspirationally trying to target a release by GopherCon 2019 (i.e., end of July). I say aspirational because there's still a number items to address.

@dsnet dsnet removed jsonpb labels Jul 10, 2019
@dsnet
Copy link
Member

dsnet commented Aug 7, 2019

Closing as this is unlikely relevant in light of the v2 re-write.

@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants