-
Notifications
You must be signed in to change notification settings - Fork 450
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
Add detection Sonos player and Sonos app #567
base: master
Are you sure you want to change the base?
Conversation
family: 'iPhone' | ||
brand: 'Apple' | ||
model: 'iPhone7,1' |
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.
Shouldn't this be only iPhone7
?
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 pulled it from here: https://docs.sonos.com/docs/soap-requests-and-responses#user-agent-header
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.
Sure thing, my question was whether we want the model to be iPhone7
or iPhone7,1
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.
Well, I'm not really sure what iPhone7,1 even means.. but I'll change it
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.
Tests are failing, mind having a look?
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.
Okay, sorry for the delay.
So, I reverted the model back to expect it as "iPhone7,1" since:
A) that's the literal example on the Sonos docs site
B) the general parser is correctly parsing the model as "iPhone7,1" and it the tests fail when I muck with it in the way suggested (they now pass).
and
C) I see precedent for both versioned and unversioned model values, depending on whether the UA contained it.
For example, from test_device.yaml
user_agent_string: 'Mozilla/5.0 (iPhone; U; fr; CPU iPhone OS 4_2_1 like Mac OS X; fr) AppleWebKit/533.17.9 (KHTML, like Gecko) Version/5.0.2 Mobile/8C148a Safari/6533.18.5'
parses as model: 'iPhone'
and
user_agent_string: 'Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0 [FBAN/FBIOS;FBAV/7.0.0.17.1;FBBV/1325030;FBDV/iPhone6,2;FBMD/iPhone;FBSN/iPhone OS;FBSV/7.0.6;FBSS/2; FBCR/Telekom.de;FBID/phone;FBLC/de_DE;FBOP/5]'
parses as model: 'iPhone6,2'
and many more examples.
Is that acceptable?
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.
sgtm, @migueldemoura what do you think?
This reverts commit cef32e4.
Support for devices from: https://www.sonos.com