Skip to content

Conversation

@humbertogontijo
Copy link
Collaborator

No description provided.

@humbertogontijo humbertogontijo self-assigned this May 5, 2023
@humbertogontijo
Copy link
Collaborator Author

@Lash-L I invited you as a colaborator so I can add you as reviewer in future changes. Could you review this one?

Copy link
Collaborator

@Lash-L Lash-L left a comment

Choose a reason for hiding this comment

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

I think this all looks good except my one typing comment.

Perhaps it makes sense to make Another dataclass called ModelStatus or something that the others inherit from, and it has all of the possibilities set to None. So you don't have to super repeat all of the non existent attributes for each one.

@humbertogontijo
Copy link
Collaborator Author

I think this all looks good except my one typing comment.

Perhaps it makes sense to make Another dataclass called ModelStatus or something that the others inherit from, and it has all of the possibilities set to None. So you don't have to super repeat all of the non existent attributes for each one.

What would this ModelStatus looks like?

@Lash-L
Copy link
Collaborator

Lash-L commented May 5, 2023

What would this ModelStatus looks like?

I realized I was wrong.

Status already has these enums as None, so they don't need to be redeclared as None as they are inherited as None from Status.

@humbertogontijo
Copy link
Collaborator Author

What would this ModelStatus looks like?

I realized I was wrong.

Status already has these enums as None, so they don't need to be redeclared as None as they are inherited as None from Status.

The only way I can think of is creating a mixing for each model status

@Lash-L
Copy link
Collaborator

Lash-L commented May 5, 2023

Sorry I think I wasn't very clear. I think how you have it is actually fine.

The base Status class already has 'fan_power' for instance, and it is set to None.

The specific S6PureStatus for example, just has to set the type of 'fan_power'

Then since the base Status class already has 'mop_mode' and it is None, S6PureStatus will also make it None.

I think the approach you have now is completely fine unless I am missing something

@humbertogontijo
Copy link
Collaborator Author

I wanted to make these fields required. But I think I'll just let it be optional for now

@humbertogontijo humbertogontijo merged commit 8092b67 into main May 5, 2023
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.

3 participants