Allow to configure bridge #49

jtyr · 2021-10-14T01:32:20Z

- 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)

Signed-off-by: Jiri Tyr <jiri.tyr@gmail.com>

amimof · 2021-10-16T07:07:46Z

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

jtyr · 2021-10-16T22:28:13Z

@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 · 2021-10-18T07:21:09Z

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 · 2021-10-18T12:37:14Z

@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 · 2021-10-19T07:14:42Z

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 · 2021-10-19T08:37:37Z

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)

amimof

utils.go is unessecary since it's only used in tests. Add these functions as private and in a test file.
If you're updating Config & SwUpdate types, we might aswell update all types in config.go accordingly.

config.go

amimof · 2021-10-22T07:00:33Z

bridge_test.go

-	c, err := b.GetConfig()
-	if err != nil {
-		t.Fatal(err)
+	c := Config{


Please add another test that updates config using a new instance of Config instead of updating. Leaving the current test alone.

There is only one instance of Config now so using Config with non-pointer values will fail.

b.GetConfig() would still return an instance of Config with pointers no? So no reason to change the current test.

GetConfig returns all fields, even those that are not editable. It makes no sense to use fields that are not editable for the UpdateConfig.

It makes no sense to update the current test. Make another one covering your change

codecov-commenter · 2021-10-22T19:48:52Z

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.

jtyr

The utils.go is needed not only for tests but also for users that might use it for declaring structures without the need to create variables first:

// Without the methods from utils.go
name := "Bridge 1"
c1 := huego.Config{
  Name: &name,
}

// With the methods from utils.go
c2 := huego.Config{
  Name: huego.StrPtr("Bridge 1"),
}

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

jtyr · 2021-10-22T19:49:40Z

bridge_test.go

-	c, err := b.GetConfig()
-	if err != nil {
-		t.Fatal(err)
+	c := Config{


There is only one instance of Config now so using Config with non-pointer values will fail.

amimof · 2021-10-23T07:21:03Z

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 · 2021-10-23T07:29:12Z

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 · 2021-10-23T10:11:54Z

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 · 2021-10-25T11:25:43Z

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 · 2021-10-25T13:10:05Z

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 · 2021-10-25T13:17:45Z

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.

amimof

Put helper functions as well as their tests in huego_test.go. They don't need their own dedicated file.

amimof · 2021-10-25T13:19:58Z

bridge_test.go

-	c, err := b.GetConfig()
-	if err != nil {
-		t.Fatal(err)
+	c := Config{


It makes no sense to update the current test. Make another one covering your change

jtyr · 2021-10-25T14:11:05Z

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 · 2021-10-25T14:16:05Z

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 · 2021-10-25T15:15:52Z

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 · 2021-10-25T15:33:23Z

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 · 2021-10-25T15:47:12Z

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 · 2021-10-25T16:20:52Z

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

amimof · 2021-10-25T16:28:28Z

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 · 2021-10-25T18:03:03Z

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

Allow to configure bridge

2003159

Signed-off-by: Jiri Tyr <jiri.tyr@gmail.com>

jtyr mentioned this pull request Oct 15, 2021

omitempty on boolean attributes prevents changing them to false #45

Open

Adding missing config/touchlink

e0e98fd

amimof requested changes Oct 22, 2021

View reviewed changes

Adding missing omitempty for DeviceTypes

7cf2220

jtyr commented Oct 22, 2021

View reviewed changes

Fixing formatting

0f88b5f

Make pointer methods private

a2b29cf

amimof requested changes Oct 25, 2021

View reviewed changes

Moving pointer methods

dee9615

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to configure bridge #49

Allow to configure bridge #49

jtyr commented Oct 14, 2021 •

edited

Loading

amimof commented Oct 16, 2021

jtyr commented Oct 16, 2021

amimof commented Oct 18, 2021

jtyr commented Oct 18, 2021

amimof commented Oct 19, 2021

jtyr commented Oct 19, 2021

amimof left a comment

amimof Oct 22, 2021

jtyr Oct 22, 2021

amimof Oct 23, 2021

jtyr Oct 23, 2021 •

edited

Loading

amimof Oct 25, 2021

codecov-commenter commented Oct 22, 2021 •

edited

Loading

jtyr left a comment •

edited

Loading

jtyr Oct 22, 2021

amimof commented Oct 23, 2021

amimof commented Oct 23, 2021

jtyr commented Oct 23, 2021

amimof commented Oct 25, 2021 •

edited

Loading

jtyr commented Oct 25, 2021

amimof commented Oct 25, 2021 •

edited

Loading

amimof left a comment •

edited

Loading

amimof Oct 25, 2021

jtyr commented Oct 25, 2021

amimof commented Oct 25, 2021

jtyr commented Oct 25, 2021

amimof commented Oct 25, 2021

jtyr commented Oct 25, 2021

jtyr commented Oct 25, 2021

amimof commented Oct 25, 2021

jtyr commented Oct 25, 2021

Allow to configure bridge #49

Are you sure you want to change the base?

Allow to configure bridge #49

Conversation

jtyr commented Oct 14, 2021 • edited Loading

amimof commented Oct 16, 2021

jtyr commented Oct 16, 2021

amimof commented Oct 18, 2021

jtyr commented Oct 18, 2021

amimof commented Oct 19, 2021

jtyr commented Oct 19, 2021

amimof left a comment

Choose a reason for hiding this comment

amimof Oct 22, 2021

Choose a reason for hiding this comment

jtyr Oct 22, 2021

Choose a reason for hiding this comment

amimof Oct 23, 2021

Choose a reason for hiding this comment

jtyr Oct 23, 2021 • edited Loading

Choose a reason for hiding this comment

amimof Oct 25, 2021

Choose a reason for hiding this comment

codecov-commenter commented Oct 22, 2021 • edited Loading

Codecov Report

jtyr left a comment • edited Loading

Choose a reason for hiding this comment

jtyr Oct 22, 2021

Choose a reason for hiding this comment

amimof commented Oct 23, 2021

amimof commented Oct 23, 2021

jtyr commented Oct 23, 2021

amimof commented Oct 25, 2021 • edited Loading

jtyr commented Oct 25, 2021

amimof commented Oct 25, 2021 • edited Loading

amimof left a comment • edited Loading

Choose a reason for hiding this comment

amimof Oct 25, 2021

Choose a reason for hiding this comment

jtyr commented Oct 25, 2021

amimof commented Oct 25, 2021

jtyr commented Oct 25, 2021

amimof commented Oct 25, 2021

jtyr commented Oct 25, 2021

jtyr commented Oct 25, 2021

amimof commented Oct 25, 2021

jtyr commented Oct 25, 2021

jtyr commented Oct 14, 2021 •

edited

Loading

jtyr Oct 23, 2021 •

edited

Loading

codecov-commenter commented Oct 22, 2021 •

edited

Loading

jtyr left a comment •

edited

Loading

amimof commented Oct 25, 2021 •

edited

Loading

amimof commented Oct 25, 2021 •

edited

Loading

amimof left a comment •

edited

Loading