Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SelectionOptions type and refactor GetThermostat to use them #15

Merged
merged 4 commits into from
Jun 29, 2024

Conversation

cvvs
Copy link
Contributor

@cvvs cvvs commented Apr 21, 2024

I wanted to be able to configure GetThemostat selection data, and you have a TODO to allow for this, so I created a SelectionOption type which can be passed into GetThermostat to set specific selection options when making a request.

I modified the GetThermostat to take the options as a variadic parameter, so the change should be backwards compatible.

Originally, the option funcs were all With<SettingName> but that seemed redundant, so removed the With.

I can undo the formatting changes made by gofmt if you prefer.

Copy link
Owner

@rspier rspier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed review, April and early May were something special...

Anyway, thank you for the PR. A few very minor tweaks if you don't mind...

Originally, the option funcs were all With but that seemed redundant, so removed the With.

I agree the With is providing much value, but the option func pattern generally uses it, so in the name of consistency, maybe add them back? (Or you could probably convince me I'm being silly about that.)

I can undo the formatting changes made by gofmt if you prefer.

Yes please. I'm not opposed to the changes, but let's make them outside the scope of this PR. Also, what version of gofmt are you using? 1.22 isn't making those changes for me.

ecobee/functions.go Outdated Show resolved Hide resolved
@cvvs
Copy link
Contributor Author

cvvs commented Jun 24, 2024

I'll try to get the requested changes done as soon as I get some spare dev time.

cvvs added 2 commits June 26, 2024 17:42
* Correct gofmt changes
* Add the With prefection to option funcs
@cvvs cvvs requested a review from rspier June 26, 2024 22:50
@rspier rspier merged commit 212aacd into rspier:master Jun 29, 2024
@rspier
Copy link
Owner

rspier commented Jun 29, 2024

Merged, thank you!

In hindsight, I wonder if we should have just done WithSelection(*Selection)? as a GetThermostatOption func?

@cvvs
Copy link
Contributor Author

cvvs commented Jun 30, 2024

Funny you should say that. I was working on a mock http client this week, and after writing way too many option funcs, I ended up doing something similar with *http.Request. Had I done this this weekend, I probably would have done what you suggested.

@rspier
Copy link
Owner

rspier commented Jun 30, 2024

It's not too late to change our mind and switch the interface.

I figure we can retain a default Selection for the common case, but anyone who wants something special can specify all the fields they care about.

@cvvs cvvs deleted the f/add_get_thermostate_options branch July 3, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants