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

Array strings #180

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Array strings #180

wants to merge 7 commits into from

Conversation

floren
Copy link
Contributor

@floren floren commented Oct 25, 2019

Description:
In master, EachKey cannot extract from an array of strings. Specifying foo.[0] would return nothing for {"foo": ["a","b"]}, because EachKey calls the Get function twice and in the case of strings inside arrays, the first Get strips quotes and the second then cannot recognize that it has been passed a string.

This PR appears to make extraction from arrays of strings work properly without breaking any existing tests. It includes a test which failed on master but passes with the changes.

Benchmark before change: Neither benchmark worked:

$ go test -test.benchmem -bench JsonParser ./benchmark/ -benchtime 5s -v
go: directory benchmark is outside main module
$ make bench
docker run -v `pwd`:/go/src/github.com/buger/jsonparser -i -t jsonparser go test  -test.benchmem -bench JsonParser ./benchmark/  -benchtime 5s -v
Unable to find image 'jsonparser:latest' locally
docker: Error response from daemon: pull access denied for jsonparser, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.
See 'docker run --help'.
make: *** [Makefile:16: bench] Error 125

@floren
Copy link
Contributor Author

floren commented Aug 7, 2020

PTAL

@floren
Copy link
Contributor Author

floren commented Jul 27, 2021

PTAL.

@cedecegreen
Copy link

Hello, Is there anything I can do to help get this PR merged?

@john-floren-gravwell
Copy link

PTAL

@kris-watts-gravwell
Copy link

any update here?

I am using a fork with the above patch, would love to remove a replace directive from my go.mod if possible. The code looks solid and I have tested it a ton.

@slvlirnoff
Copy link

Do we know why this is left open? Seems that it would be a fix for this: grafana/loki#9179

@MichelHollands
Copy link

@buger Hello, can you have a look at this fix? Thanks in advance

@jrgvf
Copy link

jrgvf commented Jan 25, 2024

Hello, any news?

@john-floren-gravwell
Copy link

john-floren-gravwell commented Jul 12, 2024

@buger This PR being now almost 5 years old, I'm making one last check to see if you have any interest in merging it; if not, I'll transition our code to using my fork. Thank you!

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

Successfully merging this pull request may close these issues.

8 participants