Add support for watering reports. #101
Conversation
thomaskistler
force-pushed
the
tkistler/reports
branch
from
c4fb49f to
7fef9e3
Compare
pydrawise/schema.py
Outdated
| options: list[Option] = field(default_factory=list) | ||
|
|
||
|
|
||
| class AdvancedProgramScopeEnum(Enum): |
There was a problem hiding this comment.
Do you actually need this enum for something? It doesn't look very useful.
The only things that need to be defined in this file are fields that we actually want returned by the client.
There was a problem hiding this comment.
Hard to tell whether someone will be able to use it. I'm assuming it's there for a reason.
There was a problem hiding this comment.
Sure, there's lots of stuff exposed by the GraphQL API, but I don't intend to expose all of it unless there are use-cases for it. There's a bunch of fields that are only useful for the Hydrawise app, for example.
There was a problem hiding this comment.
Another note here is that it's easy to add functionality when we find use-cases for it, but it's very hard to remove it.
| @@ -419,7 +608,7 @@ class Controller: | |||
| sensors: list[Sensor] = field(default_factory=list) | |||
| zones: list[Zone] = field(default_factory=list) | |||
| permitted_program_start_times: list[ProgramStartTime] = field(default_factory=list) | |||
| status: Optional[ControllerStatus] = None | |||
| status: ControllerStatus = None | |||
There was a problem hiding this comment.
If this isn't optional then it shouldn't have a default of None.
There was a problem hiding this comment.
I did this for consistency reasons. This looks to be the convention throughout this file. There are many instances of required fields that have a default. If we want to go this route, we should probably clean it up everywhere.
There was a problem hiding this comment.
If it needs a default, then it should create an empty ControllerStatus. It's a typing violation to assign None here as it's not marked optional.
If this is incorrect elsewhere here, let's fix that in a separate PR.
|
Looks like there's merge conflicts that need to be addressed. |
thomaskistler
force-pushed
the
tkistler/reports
branch
from
1958f77 to
18ceb3e
Compare
|
Resolved the merge conflict. |
|
Don't land it yet. I think you were right in that I'm going to remove some of the fields and classes to only return the data that is potentially useful. |
|
This is ready for review now. I guess once we land these changes, we will have to migrate the HA integration to the GraphQL API before we can really take advantage of it. Were you planning to do that already? |
|
I have also started working on the migration from legacy api to new graphql api in Home Assistant here: thomaskistler/home-assistant-core#1. Before I start fixing the tests, please let me know whether this approach looks ok. |
That looks really similar to what I had been working on: dknowles2/ha-core@1b7bfcc Not sure if I'll have time in the next few weeks to get that reviewed, so feel free to send the PR upstream. |
|
Ok. I will merge the two and send them upstream. Also, if you could cut a December release for pydrawise, that would be great. The most recent changes are required to integrate into HA. |
Add support for watering reports.