Conversation
BREAKING CHANGE: you can no longer import .container or .code_mappings, instead import .data
BREAKING CHANGE: you can no longer import .container or .code_mappings, instead import .data
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
LG, just two questions, can still merge and iterate if needed.
| from ..containers import RoborockBase | ||
|
|
||
|
|
||
| class dpCleanRecord(RoborockBase): |
There was a problem hiding this comment.
Interesting naming style here. Should start with capital?
There was a problem hiding this comment.
Yes- this is how it is named in the decompiled source code so I just matched. I can do typically Pythonic naming conventions if you think that is better. I went back and forth
There was a problem hiding this comment.
Will follow up based on what you say - for now will merge as this isn't actually utilized anywhere yet
There was a problem hiding this comment.
Yeah i'd say do the pythonic naming in a followup.
| @@ -0,0 +1,9 @@ | |||
| """This module is meant to hold dataclasses and codemappings for various devices and protocols.""" | |||
|
|
|||
| from .b01_q7 import * | |||
There was a problem hiding this comment.
These will all fit in the same namespace w/o any overlap? I'm a little unsure when we should import from .data vs importing from subdirectories.
There was a problem hiding this comment.
My thought was you should typically import from the subdirectories. But that felt like a very big breaking change that we could maybe make later. For now, none of them overlap namespace wise but definitely a good future concern. Because they have shared a file in the past it hasn't been a problem as I would name them differently (I.e. ZyadStatus vs Status) but now that they are in different files it would be an easier mistake to make.
I think removing the * import would be a good future step. And encouraging specific imports from the subdirectories. I think for now we have a lot of moving parts as is, so I wanted to leave it simple and similar to how it is now
2 participants
Giant change...
I was getting frustrated with the size and chaos that was .containers and .code_mappings. Let me know your thoughts here, this is a breaking change, but it is very easy to fix. Replace instances of roborock.containers or roborock.code_mappings with roborock.data