Add a new CreateUser function to return CleintKey

[SteelPhase]

There were some go format changes, and some test updates. I've also updated CreateUser to not generate a client key, as it doesn't return it, and it's never used.

Fixes #24

[SteelPhase]

Looks like something changed after go 1.14 that broke some unit tests. I fixed them, and they pass for 1.14, and 1.15 now. Any suggestions on how to resolve issues with 1.13

[amimof]

Hi @SteelPhase thanks for contributing. I made a couple of comments to your changes, have a look. I believe we can re-use the existing Whitelist struct as well as making the addition of CreateUser more specific to its purpose.

[SteelPhase]

Unbroke the tests, and fixed the versions of go tested. Updated the name for CreateUser2. Let me know what you think

[codecov-io]

Codecov Report

Merging #25 into master will decrease coverage by 4.28%. The diff coverage is 62.96%.

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   71.36%   67.07%   -4.29%     
==========================================
  Files           5        5              
  Lines        1299     1145     -154     
==========================================
- Hits          927      768     -159     
  Misses        205      205              
- Partials      167      172       +5     

Impacted Files	Coverage Δ
bridge.go	59.18% <62.96%> (-4.23%)	:arrow_down:
huego.go	74.54% <0.00%> (-3.06%)	:arrow_down:
group.go	100.00% <0.00%> (ø)
light.go	100.00% <0.00%> (ø)
scene.go	100.00% <0.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 b4a8f71...1e8caa5. Read the comment docs.

[amimof]

@SteelPhase Please have a look at my review comments. Something else I want to add is that after your changes, the current CreateUser might as well make use of CreateUserWithClientKey but just return the Username. Right now they both do the same thing but return different types.

[SteelPhase]

I see what your are saying. I'm going to take an alternate approach. let me know what you think

[amimof]

Should be quite simple really. Your new functions should basically replace the current CreateUser* functions and. And for example CreateUserContext would then do something like:

func (b *Bridge) CreateUserContext(ctx context.Context, n string) (string, error) {
    w, err := CreateUserWithClientKeyContext(ctx, n)
    if err != nil {
        return err, ""
    }
    return w.ClientKey, nil
}

And we can figure out the rest from there

[SteelPhase]

I just pushed a new commit. Tried to make it as clean as possible. I am going to test them in a few to see if they work as expected with my hue bridge. Just tied up in a meeting

[SteelPhase]

I just tested this with the bridge. CreateUser, and CreateUserWithClientKey work as expected.

[amimof]

I almost got it. Look at my example. Let your new functions do what CreateUser and CreateUserContext do today. You've basically done it the other way around. This way we don't break stuff for other users which are using CreateUser functions currently. You can ping me on the Kubernetes slack if you have any further doubts.

[amimof]

@SteelPhase consider this

func (b *Bridge) CreateUser(n string) (string, error) {
    return b.CreateUserContext(context.Background(), n)
}

func (b *Bridge) CreateUserContext(ctx context.Context, n string) (string, error) {
    w, err := b.CreateUserWithClientKeyContext(context.Background(), n)
    if err != nil {
        return nil, err
    }
    return w.ClientKey
}

func (b *Bridge) CreateUserWithClientKeyContext(ctx context.Context) (*Whitelist, error) {
    var a []*APIResponse

    body := struct {
        DeviceType        string `json:"devicetype,omitempty"`
        GenerateClientKey bool   `json:"generateclientkey,omitempty"`
    }{n, true}

    ...

    return &wl, nil
}

func (b *Bridge) CreateUserWithClientKey(deviceType string) (*Whitelist, error) {
    return b.CreateUserWithClientKeyContext(context.Background(), deviceType)
}

[SteelPhase]

I don't see how what you've posted as an example is any different than what I've committed. I have the logic in a private function, which is no different from what you have done. Your CreateUserContext would also literally break any person using the CreateUser function, as it's returning the wrong thing. You should be returing wl.Username...

[amimof]

If you don’t see it then i can’t help you any further unfortunately. The code I suggested was merely an example of the structure. So of course you might expect typos in the details.

[SteelPhase]

you aren't explaining your self, and there's no obvious difference. My public functions are the correct versions of what you are suggesting. I feel like you are missing something.

[amimof]

I’ve explained in every way possible and even given you two different code examples. There are differences in terms of code design and DRY:nes which your contribution lacks.

[SteelPhase]

There's nothing repetitive in what I did. I'm sorry you didn't bother to actually look at what I wrote. Feel free to implement your self. Just trying to help out a project, but you've made it perfectly clear you don't want it.