API changes proposal #37

gabriel-alecu · 2014-07-14T16:38:47Z

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 Map/MustMap/Array/MustArray return *Json instead of interface{} #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 Must* methods are inconsistent with the standard library. #15)

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

…arrays

mreiferson · 2014-07-15T15:55:02Z

simplejson.go

+// using GetIndex().
+//
+//   js.GetPathAny("top_level", "entries", 3, "dict")
+func (j *Json) GetPathAny(branch ...interface{}) *Json {


why not just update GetPath to behave like this, it won't be backwards incompatible...

Actually I tried that at first, but one of the tests gave an error about converting []string to []interface{}, so it's not fully compatible.

I see, when passing v... as the argument where v is []string, hmmmm

mreiferson · 2014-07-15T15:55:46Z

looks fine otherwise, API bloat aside.

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

gabriel-alecu · 2014-07-15T16:01:25Z

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 · 2014-07-15T16:05:03Z

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

gabriel-alecu · 2014-07-15T16:06:23Z

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

- Get() now works like the old GetPathAny() - Reworked CheckGet() to also take a path - Removed GetIndex(), GetPath() and GetPathAny() because the new Get() supersedes them.

Too specific, should use Array() or JsonArray() instead, and then cast the individual values as the type that you need.

- Added "Check" prefix to the ones that also return an error. - Removed the "Must" prefix from those that always return a valid value. fixes issue bitly#15

gabriel-alecu · 2014-07-17T16:55:16Z

Alright, Updated the first comment with more changes.

mreiferson · 2014-07-18T10:12:17Z

nice work @AzuraMeta

another naming suggestion. I think Json should be JSON.

gabriel-alecu · 2014-07-18T10:33:08Z

Done :)

mreiferson · 2014-07-18T10:51:25Z

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.

It's not a cast, but a straight-up conversion, with limited uses. Also, fixed JSON*() descriptions.

gabriel-alecu · 2014-07-18T11:05:52Z

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 · 2014-07-18T11:57:24Z

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

gabriel-alecu · 2014-07-18T15:13:09Z

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 · 2014-07-18T16:13:55Z

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 · 2014-07-19T20:15:21Z

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:

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.
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 · 2014-07-21T08:38:44Z

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

gabriel-alecu · 2014-07-21T08:40:49Z

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

gabriel-alecu · 2014-07-25T09:20:29Z

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 · 2014-07-25T12:03:30Z

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

xyproto · 2015-06-16T12:46:39Z

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.

AzuraMeta added 3 commits July 14, 2014 13:59

Added GetPathAny(), which is like GetPath(), but can also go through …

6505a08

…arrays

Added MapJson() and ArrayJson()

110b96b

Some naming and comment edits.

287f6fb

mreiferson reviewed Jul 15, 2014
View reviewed changes

AzuraMeta added 5 commits July 16, 2014 10:41

Added MustJsonMap() and MustJsonArray()

70959dc

(COMP BREAK) Reworked the Get() methods

bb58a70

- Get() now works like the old GetPathAny() - Reworked CheckGet() to also take a path - Removed GetIndex(), GetPath() and GetPathAny() because the new Get() supersedes them.

Removed StringArray()

cfb04ab

Too specific, should use Array() or JsonArray() instead, and then cast the individual values as the type that you need.

Changed the cast methods

d6f7ad3

- Added "Check" prefix to the ones that also return an error. - Removed the "Must" prefix from those that always return a valid value. fixes issue bitly#15

Fixed missing return error in go 1.0.3

f48d285

gabriel-alecu changed the title ~~GetPathAny(), JsonArray(), JsonMap()~~ simplejson2 proposal Jul 17, 2014

Removed misplaced prefix from CheckInterface()

a487d55

gabriel-alecu changed the title ~~simplejson2 proposal~~ API changes proposal Jul 17, 2014

Renamed *Json*() methods to *JSON*()

0030fe8

Removed CheckBytes()

bbcf290

It's not a cast, but a straight-up conversion, with limited uses. Also, fixed JSON*() descriptions.

Check*() methods now return a bool instead of an error

ea30edb

Fixed a missed go1.0 bug

246029b

AzuraMeta added 3 commits July 21, 2014 13:26

Renamed Json type to JSON

880249d

Renamed NewJson to NewJSON()

2a47cd5

Forgot to update the tests :P

6c0cf65

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API changes proposal #37

API changes proposal #37

gabriel-alecu commented Jul 14, 2014

mreiferson Jul 15, 2014

gabriel-alecu Jul 15, 2014

mreiferson Jul 15, 2014

mreiferson commented Jul 15, 2014

gabriel-alecu commented Jul 15, 2014

mreiferson commented Jul 15, 2014

gabriel-alecu commented Jul 15, 2014

gabriel-alecu commented Jul 17, 2014

mreiferson commented Jul 18, 2014

gabriel-alecu commented Jul 18, 2014

mreiferson commented Jul 18, 2014

gabriel-alecu commented Jul 18, 2014

mreiferson commented Jul 18, 2014

gabriel-alecu commented Jul 18, 2014

mreiferson commented Jul 18, 2014

gabriel-alecu commented Jul 19, 2014

mreiferson commented Jul 21, 2014

gabriel-alecu commented Jul 21, 2014

gabriel-alecu commented Jul 25, 2014

mreiferson commented Jul 25, 2014

xyproto commented Jun 16, 2015

API changes proposal #37

Are you sure you want to change the base?

API changes proposal #37

Conversation

gabriel-alecu commented Jul 14, 2014

mreiferson Jul 15, 2014

Choose a reason for hiding this comment

gabriel-alecu Jul 15, 2014

Choose a reason for hiding this comment

mreiferson Jul 15, 2014

Choose a reason for hiding this comment

mreiferson commented Jul 15, 2014

gabriel-alecu commented Jul 15, 2014

mreiferson commented Jul 15, 2014

gabriel-alecu commented Jul 15, 2014

gabriel-alecu commented Jul 17, 2014

mreiferson commented Jul 18, 2014

gabriel-alecu commented Jul 18, 2014

mreiferson commented Jul 18, 2014

gabriel-alecu commented Jul 18, 2014

mreiferson commented Jul 18, 2014

gabriel-alecu commented Jul 18, 2014

mreiferson commented Jul 18, 2014

gabriel-alecu commented Jul 19, 2014

mreiferson commented Jul 21, 2014

gabriel-alecu commented Jul 21, 2014

gabriel-alecu commented Jul 25, 2014

mreiferson commented Jul 25, 2014

xyproto commented Jun 16, 2015