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

Cannot get Error from callback #230

Open
yb3616 opened this issue Apr 29, 2021 · 4 comments
Open

Cannot get Error from callback #230

yb3616 opened this issue Apr 29, 2021 · 4 comments
Labels
Milestone

Comments

@yb3616
Copy link

yb3616 commented Apr 29, 2021

if e != nil {

@PiotrKozimor
Copy link

+1, for now error passed to callback will be always nil and break will never occur. I think the right api would be:

func ArrayEach(data []byte, cb func(value []byte, dataType ValueType, offset int) error, keys ...string) (offset int, err error)

@buger buger added the breaking label Jun 18, 2021
@buger buger added this to the v2 milestone Jun 18, 2021
@buger
Copy link
Owner

buger commented Jun 18, 2021

@PiotrKozimor you are right that callback at the moment never receive the error. But I guess proper way to handle it will be smth like:

Instead of

jsonparser/parser.go

Lines 1028 to 1030 in 09bcf22

if e != nil {
return offset, e
}

Do

if err != nil {
  cb(nil, t, offset+o-len(v), e)
  return offset, e
}

Well it does not make TOO much sense from API point of view, since it always returns on the first error. But at least it does not break API (I really care about backward compatibility).

I've set v2 milestone, so when we do next major release, we can "remove" err object from callback or fix it in another way.

@PiotrKozimor
Copy link

@buger It would make sense as a temporary, compatible fix.

Going back to my breaking suggestion - when error is returned from callback, iterating over array would be stopped and this error would be returned from ArrayEach. That's how I see it. Fail fast could bring benefit in some use cases.

@jmo-qap
Copy link

jmo-qap commented Oct 17, 2022

Agree with @PiotrKozimor recommendation, I would like that behavior and that would bring it inline with ObjectEach, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants