webcompat / webcompat.com

Source code for webcompat.com
https://webcompat.com
358 stars 191 forks source link

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

Open karlcow opened 6 years ago

karlcow commented 6 years ago

See the discussion in https://github.com/webcompat/webcompat.com/pull/1944#pullrequestreview-79976734

Currently we only accept MyBrowser 90.0 with this regex ([^\d]+?)\s[\d\.]+ in https://github.com/webcompat/webcompat.com/blob/aba261f71776f35348fedeca63f35eeb1e6667c6/webcompat/webhooks/helpers.py#L37-L46

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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