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

Filtering out (or not) the version-less browser name in tagging #1949

Open
karlcow opened this issue Dec 1, 2017 · 3 comments
Open

Filtering out (or not) the version-less browser name in tagging #1949

karlcow opened this issue Dec 1, 2017 · 3 comments

Comments

@karlcow
Copy link
Member

karlcow commented Dec 1, 2017

See the discussion in #1944 (review)

Currently we only accept MyBrowser 90.0 with this regex ([^\d]+?)\s[\d\.]+ in

# Only proceed if browser looks like "FooBrowser 99.0"
if browser and re.search(r'([^\d]+?)\s[\d\.]+', browser):
browser = browser.lower()
browser = browser.rsplit(' ', 1)[0]
browser = browser.encode('utf-8')
browser = browser.translate(None, '()')
dash_browser = '-'.join(browser.split())
return 'browser-{name}'.format(name=dash_browser)
else:
return None

But does it have to be like this? Do we need to drop string without version when we anyway drop the version number :O)

(also: remove this else: in there)

@miketaylr
Copy link
Member

But does it have to be like this?

I don't think there's a technical reason, I think we've just always done it that way.

@karlcow
Copy link
Member Author

karlcow commented Dec 7, 2017

@miketaylr

Could we come up with a list of strings and what we would expect for browser-*?

Currently we have

        metadata_tests = [
            ({'browser': 'Firefox'}, None),
            ({'browser': 'Firefox Mobile'}, None),
            ({'browser': 'Firefox99.0'}, None),
            ({'browser': 'Firefox (tablet)'}, None),
            ({'browser': 'Firefox 30.0'}, 'browser-firefox'),
            ({'browser': 'Firefox Mobile 30.0'}, 'browser-firefox-mobile'),
            ({'browser': 'Firefox Mobile (Tablet) 88.0'},
                'browser-firefox-mobile-tablet')
        ]

I propose we add/change
https://en.wikipedia.org/wiki/List_of_web_browsers
http://www.useragentstring.com/pages/useragentstring.php

        metadata_tests = [
            ({'browser': 'Firefox'}, 'browser-firefox'),
            ({'browser': 'Firefox Mobile'}, 'browser-firefox-mobile'),
            ({'browser': 'Firefox99.0'}, 'browser-firefox'),
            ({'browser': 'Firefox (tablet)'}, 'browser-firefox-tablet'),
            ({'browser': 'Firefox 30.0'}, 'browser-firefox'),
            ({'browser': 'Firefox Mobile 30.0'}, 'browser-firefox-mobile'),
            ({'browser': 'Firefox Mobile (Tablet) 88.0'}, 'browser-firefox-tablet'),
            ({'browser': 'Firefox Mobile Nightly 59.0a1 (2017-12-04)'}, 'browser-firefox-mobile'),
        ]

what about

another solution is to be a lot more cut in the deep.
and everything which contains firefox becomes browser-firefox
and everything which contains mobile becomes device-mobile

in that way we minimize our chances to get bruises and failures.

@zoepage
Copy link
Member

zoepage commented Dec 7, 2017

+1 on the second choice as we talk about it a few times already and the refactor is already accounting for it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants