Skip to content
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

Fixes #1030. More robust handling for UA parsing methods. #1032

Merged
merged 9 commits into from
May 2, 2016

Conversation

miketaylr
Copy link
Member

Rewrote the following methods to be more paranoid about input before passing it to ua-parser:

get_browser
get_os
get_browser_name

And added a bunch of tests.

r? @karlcow

Mike Taylor added 3 commits April 29, 2016 17:31
Give it a None default arg, in case it ever gets called without an arg.
Always return "unknown" by default.
If we get weird results back (i.e., "other"), normalize to "unknown".
Mike Taylor added 3 commits April 29, 2016 17:56
Default None arg.
Return "Unknown" by default.
Normalize "Other" to "Unknown"
Add tests.
Default None arg.
Return "Unknown" by default.
Switch position of "(Tablet)" modifier to come before the version string.
Normalize "Other" result to "Unknown" for weird values.
self.assertEqual(get_browser(None), 'Unknown')

def test_get_browser(self):
'''Test Browser parsing for non-tablet devices.'''

This comment was marked as abuse.

PARSED_NON_TABLET_BROWSER = "Firefox Mobile 40.0"
PARSED_TABLET_BROWSER = "Firefox Mobile 41.0 (Tablet)"
FIREFOX_UA = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0' # nopep8
FIREFOX_MOBILE_UA = 'Mozilla/5.0 (Android; Mobile; rv:40.0) Gecko/40.0 Firefox/40.0' # nopep8

This comment was marked as abuse.

This comment was marked as abuse.

@miketaylr
Copy link
Member Author

Thanks for reviewing! I'll push some follow up commits this afternoon or evening.

@miketaylr
Copy link
Member Author

OK, addressed the simple things and filed #1035 as a follow up bug.

r? @karlcow

@karlcow
Copy link
Member

karlcow commented May 2, 2016

r+ Yeah 🎇

@karlcow karlcow merged commit c66c939 into master May 2, 2016
@miketaylr
Copy link
Member Author

Thanks Karl!

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.

2 participants