Added ParsePrimitiveValue convenience function for users

[daboyuka]

Users will often want to convert a []byte returned by Get() or ArrayEach() that represents a primitive value (Null, Boolean, Number, String) into the corresponding Go type (nil, bool, float64, string). This PR provides a convenience method, ParsePrimitiveValue([]byte) (interface{}, error), which performs this service for such users.

Since this is just a convenience method, Get(), etc. are unchanged (except some gofmt whitespace diff in searchKeys...)

[buger]

I'm not fan of using interface{}, and would prefer avoid it as long as possible. However i understand what you trying to achieve. When you use interface{} you have to know data type anyway, so instead i propose alternative approach #29. It define separate ParseInt, ParseFloat and ParseString, so instead of:

i := ParsePrimitiveValue(value, dataType).(int)

You will do:

if (dataType == Number) {
   i := ParseInt(value)
}

It is better in my view, and more explicit.

[daboyuka]

I think that would be good idea as well, and I understand why you want to avoid interface{} (besides this function, everything else has avoided it, which is nice and clean).

However, many users are just going to throw those Parse* calls into a switch statement (I know I will). What is the harm in providing an efficient convenience method for this common use case, in addition to what you propose?

[buger]

Well i need to keep library API consistent.

When you call it using interface you should know the type, because if you provide different type program will panic. So it means that semantically ParseInt(value) is identical to ParsePrimitiveValue(value, dataType).(int) at least but way more error prone.

Maybe i'm missing the point, and if so, pls provide full featured real world example of how you see it.

[daboyuka]

The point is only convenience for a common use case, but I understand what you're saying and am not strongly pushing for this.

Here's the example from my project, modified a bit so I'm not just copying company code here :):

func (r RawJsonStructure) Lookup(pathStr string) (interface{}, bool) {
    path := strings.Split(pathStr, ".")
    vbytes, jtype, _, err := jsonparser.Get(r.raw, path...)

    if err == jsonparser.KeyPathNotFoundError {
        return nil, false
    } else if err != nil || jtype == jsonparser.Unknown { // these conditions should always happen together, but just to be safe
        panic(fmt.Sprintf("could not parse JSON in RawJsonStructure.Lookup(): %s", err))
    }

    return jsonparser.ParsePrimitiveValue(vbytes, jtype), true
}

The Lookup function is part of an established interface in this project. Basically, we're using interface{} to remember both the value and the type of things around the application, since that's one thing it's designed to do.

Again, I'm not adamant about this; I can make do with just the simpler convenience methods you suggest, and honestly I do like the simplicity of keeping everything explicit in this library. It's your call, I'm happy either way.

[buger]

Frankly i do not see need for helper Lookup function here, and in places where it is called it can be replaced with:

value, err = jsonparser.GetString(data, strings.Split(pathStr, ".")...)

# or 

value, err = jsonparser.GetInt(data, strings.Split(pathStr, ".")...)
if err != nil {
...
}

Your error checks basically do nothing, only return boolean value instead of error object.

[daboyuka]

This assumes the code already knows what the ValueType of the value is, which is not the case in my code.

As for the error checks, as I said, the interface is already defined, so I must return (interface{}, bool).

[daboyuka]

You've convinced me that the per-type parser methods (ParseInt, ParseString, etc.) are the better way for this library. I will rework this branch into that form and submit a new PR. Thanks!

[buger]

Well, when you call Lookup function inside your code, you have to get its value from interface{} anyway, nope? Can you show then how Lookup function is used.

Sorry if bothering you too much, but i'm really curious how people use library, its very important on design decisions. Thank you!

[daboyuka]

Lookup is called in many places (about 160 according to my IDE), as it is part of a fundamental abstract "row structure" interface. One of the main cases that define why Lookup must return an interface{} is that many structs in our project implement Lookup and return their fields this way. Since these fields may be of any time (int, float64, even other structures, etc.), a general Lookup function is needed.

My use of the jsonparser library is to more efficiently implement the case where a row comes from JSON, but not all rows are JSON.

[daboyuka]

I guess the bottom line is that the type isn't checked on the return value until much later, rather than in the code immediately after the jsonparser.Get() call, so the type needs to be packaged up and saved until that later time.

[buger]

I see, thank you! Let me think about it for some time :)

[daboyuka]

As I said, I am now convinced that your suggestion is the better way here. I would like to refactor this PR to use per-type ParseInt, ParseString, etc. functions, and then in my project code I will just wrap them in a simple switch statement myself. My main concern was getting access to the efficient BytesParseInt functions etc. within jsonparser, so this will achieve that goal and also keep the library clean :). I'll make a new PR and then you can choose more easily.