Allow to configure bridge

[jtyr]

- What I did Current implementation doesn't allow to configure bridge (e.g. its name). More details are in this comment.

- How I did it I have change the Config type to use pointers which sets nil value by default and gets filtered out by JSON marshaling.

- How to verify it

bridge := huego.New("192.168.x.y", "asdfasdf...")

c := &huego.Config{
    Name: huego.StrPtr("My Bridge"),
}

_, err := bridge.UpdateConfig(c)
if err != nil {
    panic(err)
}

- Description for the CHANGELOG Added the possibility update bridge's configuration (e.g. its name)

[amimof]

Hi @jtyr I posted an alternative solution in #45 which doesn’t break compatibility which is important. Let me know what you think.

[jtyr]

@amimof Not sure how your proposed solution would allow the user to submit only defined values to the bridge. Please could you elaborate a little bit more on your solution and perhaps compare it to the solution I'm proposing in this PR?

[amimof]

Although your contribution solves the problem, unfortunately it breaks compatibility since the API has changed requiring different inputs in the form of pointers.

To get around this, each struct (in this case Config) can implement a JSON marshaler with an intermediate struct containing fields with pointers, allowing booleans and strings to be nil. Thus fixing the problem. For example:

func (c *Config) MarshalJSON() ([]byte, error) {

    type Alias Config
    d, err := json.Marshal(&struct{
        *Alias
        Name             *string               `json:"name,omitempty"`
        APIVersion       *string               `json:"apiversion,omitempty"`
        SwVersion        *string               `json:"swversion,omitempty"`
        ProxyAddress     *string               `json:"proxyaddress,omitempty"`      
        ....
    }{
        Alias: (*Alias)(s),
        Name: &c.Name,
        APIVersion: &c.APIVersion,
        SwVersion: &c.SwVersion,
        ProxyAddress: &c.ProxyAddress,
    })
    if err != nil {
        return nil, err
    }

    return d, nil
}

[jtyr]

@amimof I have just tested your solution and it doesn't seem to work (the final JSON still contains fields that I haven't define in my Config instance): https://play.golang.org/p/DNqwm0AayNr

Please could you explain what's the Alias for?

[amimof]

Yes, you are right, my proposal fixes the nullified fields problem but doesn't allow for updating fields selectively. I'm trying out another approach where types have accessor methods in order to update the fields. I'll get back to you once I have a working example.

[jtyr]

We could create new type with pointers (e.g. Copy - old, and CopyPtr - new) and change the UpdateConfig method to accept an interface{} instead of Config and then if the user passes the Copy type, we could do the translation to CopyPtr type. Or just wait few months for Go v1.18 where we gonna have generics ;o)

[codecov-commenter]

Codecov Report

Merging #49 (dee9615) into master (f543730) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #49   +/-   ##
=======================================
  Coverage   73.53%   73.53%           
=======================================
  Files           5        5           
  Lines        1413     1413           
=======================================
  Hits         1039     1039           
  Misses        207      207           
  Partials      167      167           

Impacted Files	Coverage Δ
bridge.go	63.80% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f543730...dee9615. Read the comment docs.

[amimof]

I understand that it's convenient but those kind of helper functions are so trivial that they might as well be created by the user. In our case we use it internally for tests so there is no reason for exposing them as part of the API.

[amimof]

It's not required to update all types in config.go as only those that are editable should use pointers.

I'm planning on implementing the same approach for all types. Which requires me to go through the Hue API documentation and updating each type appropriately.

[jtyr]

I understand that it's convenient but those kind of helper functions are so trivial that they might as well be created by the user. In our case we use it internally for tests so there is no reason for exposing them as part of the API.

Tests might be a source of examples for users. So if we use it for testing, users should be able to use them in their code as well. Why should users create their implementation of those functions if they could come with Huego?

[amimof]

Internal functions are not exposed to users for this reason. As I said, the helper functions are very trivial. A function that returns a pointer to an argument will not be a part of Huego.

[jtyr]

I have made the pointer methods internal. But as a user of this API implementation, I'm really not happy that you don't provide me with this functionality and that you force me to implement it again on my side if I want to use your package!

[amimof]

I'm not forcing you to do anything. You may use whatever library you want for this purpose. Or whatever Hue API implementation for that matter. I don't want to include code that is not part of the project.

[jtyr]

I'm not forcing you to do anything. You may use whatever library you want for this purpose. Or whatever Hue API implementation for that matter.

By not exporting the pointer methods you indirectly forcing users like me to implement those methods by themselves. You would make the package more user-friendly by exporting the pointer methods.

I don't want to include code that is not part of the project.

If you merge this PR then the code will be part of the project. It's only about to export the methods and make them available to the users.

[amimof]

It’s literally 3 lines of code. You mean it’s too difficult for you so you need a library to do that for you?

[jtyr]

It’s literally 3 lines of code. You mean it’s too difficult for you so you need a library to do that for you?

It's more about how convenient instead of how difficult it's. Me, as an user of this package, I would really appreciate to have this functionality provided instead of having to develop it by myself. It's also convenient for you when you are documenting examples of how to use this package. How are you gonna document the need to use pointers for Update methods? Are you gonna define variables first and then use their reference? That an extra line of code you have to write. Simpler would be to use the built-in method to turn the value to pointer.

[amimof]

I’d rather avoid bloat the codebase than, providing convenience which in this case I argue that these functions do not. In any case I’m sure there are several libraries providing this functionality. I as a user appreciate clean code which does what it’s intended to do, nothing more nothing less. Following the principle “Do one thing and do it well”.

[jtyr]

If there is such library then let's use it instead of adding those methods here. Please point me to such library and I will amend the PR accordingly.

[jtyr]

What about https://github.com/AlekSi/pointer?

[amimof]

You are free to use whatever library you want alongside Huego. I took the liberation to create a library for your convenience:

https://github.com/amimof/ptr

[jtyr]

Good stuff. Let's shorten the names if it's already in the ptr package. I have created PR for that here: https://github.com/amimof/ptr/pull/1