API changes proposal

[gabriel-alecu]

Edit: No point in closing and re-opening the pull request, I'll just generalize this one.

So, I have a few suggestions for the next version of the API:

API bloat reduction:

• Replaced all the Get*() methods with a single one which can follow a path through both maps and arrays.
• Removed StringArray() converter (too specific, you can use the new JSONArray() method and then cast each value as whatever you want)
• Removed Bytes() because it wasn't a cast but a straight up conversion, with very limited uses.

Navigation improvement:

• Added JSONMap() and JSONArray(), which return map and array values as JSONs. (fixes issue #25)

Naming conventions:

• Casting methods which could return an error now use the Check* prefix and return a bool indicating success. Instead, those that always returned a value no longer have the Must* prefix. (fixes issue #15)

For reference, doc link for the modified API: http://godoc.org/github.com/AzuraMeta/go-simplejson

[mreiferson]

looks fine otherwise, API bloat aside.

We really need to think about go-simplejson 2 at some point :grin:

[gabriel-alecu]

I'm currently using this extensively in a project, and feel the need for a few changes. I could give a hand with a possible simplejson2.

[mreiferson]

How about writing up an issue with your proposed changes/API here on this repo so we can plan the next version?

[gabriel-alecu]

I don't have anything clear just yet, but I'll see what I can do.

[gabriel-alecu]

Alright, Updated the first comment with more changes.

[mreiferson]

nice work @AzuraMeta

another naming suggestion. I think Json should be JSON.

[gabriel-alecu]

Done :)

[mreiferson]

also, since we only really ever generate our own errors, it's possible that the Check* methods should return (*JSON, bool) rather than (*JSON, error), since it isn't incredibly useful.

[gabriel-alecu]

I also thought of that, but CheckUint64() uses strconv.ParseUint() which can return a more complex error.

Maybe we should change that instead, using string for a number conversion is weird in the first place...

[mreiferson]

It needs to use that for big numbers, I think.

[gabriel-alecu]

Alright did it anyway, I simply ignored the error details in those specific cases.

PS: When we're all done with the changes, I'm gonna re-organize the code a bit, but until then I don't wanna mess-up the diff too much.

[mreiferson]

this is looking great!

FWIW I meant all instances of Json to JSON. This made me realize one path forward - we could keep both types in this package (the Json type would be the legacy API and the JSON type would be the new v2 API).

This would give people the opportunity to transition seemlessly for eventual deprecation of the old API.

Thoughts @jehiah

[gabriel-alecu]

If you're suggesting to bundle both versions in the same package, I disagree. Instead of one bloated api you'd get a huge api with 2 sub-apis, and new people would be confused as to which they should use.

I think the best course would be to have the new api in a different repo, with a link at top of this one's readme. That way existing users could safely update, while new ones will see the new version right away.

Also, I'm not sure how to handle the remaining issues:

1. Get() and Set() don't work in the same way. I can't make Set() be the same as Get() because of arrays, something like js.Set("list", 3, "test") could imply adding empty values. The easiest way to fix this would be to change one or both's names so they don't look like they should work in the same way.
2. A problem with JSONMap() and JSONArray() Let's take the following example:

fmt.Println(js.Get("a_list").Array()) //let's say it would print 2 elements
lsj := js.Get("a_list").JsonArray()
lsj = append(lsj, simplejson.New())
js.Set("a_list", lsj)
fmt.Println(js.Get("a_list").Array()) //this will print empty

The cause for this is that Array() doesn't recognize []_Json as a valid array. And it's not just Array() and Map(). One way to fix this would be to have Set() convert []_Json and map[string]*Json back into []interface{} and map[string]interface{}.

[mreiferson]

1. I don't think Set needs to mirror the "path" functionality of Get, if that's what you mean.
2. I think you are correct, Set needs to "unpack" *Json

[gabriel-alecu]

1. Then how about removing the incomplete SetPath() instead? It only does half the job, and with a weird syntax as well.

[gabriel-alecu]

Hey I was wondering, should the Int(), Float64() etc..., convert from strings? I saw there are a lot of cases, but none for string. So I can do something like:

jsStr := `{"test":"24"}`
js, _ := simplejson.NewJSON([]byte(jsStr))
fmt.Println(js.Get("test").Int()) //this would currently print 0

[mreiferson]

I don't think so, but I'm open to other opinions

[xyproto]

Forked go-simplejson, incorporated most of the changes from this pull request and added a few extra functions for manipulating JSON files. If it's of any interest, the resulting repository is here.