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

Revised implementation of enhancement: add temp reading of fritz box … #11

Closed
wants to merge 2 commits into from

Conversation

mwittig
Copy link
Contributor

@mwittig mwittig commented Jun 12, 2015

#2, includes fixes for: No values anymore after update #7

@plugin.fritzCall("getSwitchEnergy", @config.ain)
.then (energy) =>
@_setEnergy(Math.round(energy / 100.0) / 10.0)
.then (state) =>
Copy link
Owner

Choose a reason for hiding this comment

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

why is this updated/ outdented now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, my IDE (IDEA) just did this. It should not have any impact as the line is part of a chained call. Feel free to recover the original ident. Note however, that the following two then() class aren't indented on master, either! See https://github.com/andig/pimatic-fritz/blob/master/fritz.coffee#L156 and follwing three lines! So, indentation on master varies.

@andig
Copy link
Owner

andig commented Jun 15, 2015

Are you confident now that this works for everybody?
Are the fixes from https://github.com/andig/pimatic-fritz/pull/10/files no longer needed?

@mwittig
Copy link
Contributor Author

mwittig commented Jun 15, 2015

I have tested both update modes thoroughly. Indentation changes have no impact on semantics (see comments on diff). Code changes of PR#10 are included in the current PR.

@andig
Copy link
Owner

andig commented Jun 15, 2015

Could you move the PR10 changes to smartfritz? As it helps abstacting the basic fb api I believe thats were it belongs. Fine to merge then. As for indentation I prefer it unchanged if code is not touched but I realize that IDEs may have differing options.

@mwittig
Copy link
Contributor Author

mwittig commented Jun 19, 2015

@andig happy to do that eventually, but I don't have much time right now. So, it'll be great if you can accept the PR as is for now. Regarding smartfritz I am wondering whether or not @nischelwitzer is still maintaining smartfritz. Did you consider contributing your code changes back to the original repo?

@andig
Copy link
Owner

andig commented Jun 25, 2015

Regarding smartfritz I am wondering whether or not @nischelwitzer is still maintaining smartfritz. Did you consider contributing your code changes back to the original repo?

Tried to, but never got a response: nischelwitzer/smartfritz#1. As smartfritz-promise has a different api it doesn't make much sense anyway.

happy to do that eventually, but I don't have much time right now. So, it'll be great if you can accept the PR as is for now.

Same for me... Please move the one function to https://github.com/andig/smartfritz

@andig
Copy link
Owner

andig commented Sep 8, 2015

ping @mwittig

@mwittig
Copy link
Contributor Author

mwittig commented Sep 11, 2015

Ah, ok. Sorry for not looking into this any earlier. Hope, I find some time to get this done next week.

@andig
Copy link
Owner

andig commented Nov 6, 2015

I've added a TemperatureSensor based on the XML api that will be able to read temps of both switches and thermostats. This PR is no longer needed.

@andig andig closed this Nov 6, 2015
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