-
Notifications
You must be signed in to change notification settings - Fork 155
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
Group support #120
Group support #120
Conversation
..so that it returns zeroconf data instead.
..as we can now not be shure whether device data was obtained via DIAL or zeroconf.
@skorokithakis |
Hmm, why would upgrades be an issue? We'd just need to make a note somewhere near the cache to know what kind of information we store, so we can migrate it (or just delete it) between versions. It might not be such a bad idea to key the cache off the app version, if we ever introduce a backwards-incompatible schema change. |
OK, so we implement a check of the cache format and delete if it's wrong?
Please explain. |
Add the app's version to the keys of the cache (or the cache dir), so if the user upgrades the app to |
Ah ok, thanks. |
..that also stores ports for groups.
It works! Thanks! |
@skorokithakis Did you forget about this? |
No, it's still WIP so I thought you wanted to add something. |
Ah, didn't think about that. It's ready for review. |
catt/controllers.py
Outdated
@@ -184,25 +189,39 @@ def clear(self): | |||
|
|||
class Cache(CattStore): | |||
def __init__(self): | |||
cache_path = Path(tempfile.gettempdir(), "catt_cache", "chromecast_hosts") | |||
cache_path = Path(tempfile.gettempdir(), "catt_%s_cache" % CATT_VERSION, "chromecast_hosts") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you sha1 the version here and get the first 8 chars or so? It's not very likely, but I'd like to guard against a version containing slashes like 0.4.3-pre1/amd
or whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
catt/controllers.py
Outdated
fetched = data[min(data, key=str)] | ||
return (fetched["ip"], fetched.get("group_port")) if fetched else (None, None) | ||
|
||
def set_data(self, name, ip, port): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're 3-only now, can you add some types here? It helps with mypy. I'll add pre-commit so this is checked on CI as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh we already added pre-commit
, I'll see about adding mypy to it. The problem with having tens of projects is that you forget what you did where :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had to comment out the baseclass set_data
method in order to avoid getting this mypy
error:
catt/controllers.py:221: error: Signature of "set_data" incompatible with supertype "CattStore"
(adding -> None
does not help)
I realize that this might be bad OOP design anyway, so I'd appreciate any suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a mypy bug. I added a type ignore on those lines for now.
..problematic baseclass definition (mypy complains) temp. commented out.
catt/controllers.py
Outdated
@@ -221,7 +240,7 @@ def __init__(self, state_path, mode): | |||
elif mode == StateMode.ARBI: | |||
self._write_store({}) | |||
|
|||
def get_data(self, name): | |||
def get_data(self, name: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't intend for this to be included in the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the change to this line.
The |
Arghhhh |
We are now assuming that 1st/2nd gen. chromecasts are the only kinds of devices that report different names via DIAL/zeroconf.
fixes #119
Please test, as I do not have access to multiple audio devices myself.