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

Dead / Faulty code in ArrayEach? #255

Open
Bios-Marcel opened this issue Oct 3, 2022 · 2 comments
Open

Dead / Faulty code in ArrayEach? #255

Bios-Marcel opened this issue Oct 3, 2022 · 2 comments

Comments

@Bios-Marcel
Copy link

Hey,

I've been trying to use ArrayEach to parse an array. The callback you can pass takes an error value as its argument and can't return one, meaning that even if you want to stop iterating, you can't. Additionally, the passed error can't be non-nil.

v, t, o, e := Get(data[offset:])

if e != nil {
	return offset, e
}

if o == 0 {
	break
}

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

if e != nil {
	break
}

If the error is not nil, we'll run into return offset, e. Since e isn't being reassigned before or after the call to cb, as go does't allow this with type error (value type), the if e != nil { break } is dead code and err will always be nil inside of the callback.

What were the thoughts behind the API design of this function? If I don't understand it, I think comments would be really helpful, both inline and function docs.

@80avin
Copy link

80avin commented Oct 7, 2022

The API is really uncomfortable to write and I also faced the exact same issue.

I opened the #253 for the same issue and also raised the PR #254 to show a proof of concept.

The API proposed is like

	next, err := jsonparser.ArrayIterator(data, "menu", "items")
	if err != nil {
		// handle error
	}
	for v, t, o, e := next(); e == nil; v, t, o, e = next() {
          // do anything or break when required
	}

If that interests you, you can support & suggest your feedback on that also.

@jmo-qap
Copy link

jmo-qap commented Oct 17, 2022

I've also encountered this very nearly bug #230 for additional context since it's not quite obvious.

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

No branches or pull requests

3 participants