dom icon indicating copy to clipboard operation
dom copied to clipboard

Simplify signatures in js.go

Open smoyer64 opened this issue 5 years ago • 2 comments

In PR #41 I talked about simplifying the signatures of several methods in js.go but rejected that as I thought it would be a breaking change. Since then, I've realized that because the first parameter is the same type as the second variadic parameter, the change is not in fact breaking and simplifies the signature of the end-user API.

I'd suggest that the following three method signatures be changed below:

  • From Get(name string, path ...string) to Get(name ...string
  • From Class(class string, path ...string) to Class(class ...string)
  • From (v Value) Get(name string, path ...string) to (v Value) Get(name ...string)

The change to the Class method slightly simplifies the key generation but otherwise the code is pretty much the same. The main benefit is that, having a single parameter, it makes more sense when a nested class is specified. Consider Class("mdc", "chips", "MDCChipSet") as described in my previous example - with the original signature, the class parameter would contain mdc but the actual class (MDCChipSet) being loaded would be the second element of the path slice. With the revised signature, the elements make sense in the order they're provided.

I do realize this is a completely cosmetic change but wanted to present the idea prior to the library reaching 1.0. If it sounds reasonable, I've got a branch I used to test that this could be done simply - assign this to me and I'll clean up that branch and submit a PR.

smoyer64 avatar May 28 '19 12:05 smoyer64

You are totally right about this not being a breaking change.

Although I initially decided to go the string, ...string way to make sure that the compiler catches things like Get() with no arguments. I think this kind of safety precaution may be important for this low-level API.

At the same time, this API only makes sense for the static set of names (consts). For example, if you try to do something like path := []string{...}; v.Get(path...) it won't work, because the call expects the first argument separately. v.Get(path[0], path[1:]...) works though, so it may be a reasonable tradeoff for catching the Get() call (and Class() now) at the compile time instead of runtime.

Still open for the discussion on changing this :slightly_smiling_face:

dennwc avatar May 28 '19 13:05 dennwc

As I noted ... I just wanted to at least talk about it. One thing I didn't think about was that my test code will currently panic if no arguments are passed (since zero is not a valid index for an empty array). That probably negates the very slight simplification of the implementing code. I guess only real question is whether it's worth making the API simpler in exchange.

I'll leave this up to you ... and there's no hurry either way so we can mull it over as long as you want.

smoyer64 avatar May 28 '19 14:05 smoyer64