Fixes #1035 - Unified code for version string

[moraveyo]

r? @miketaylr

[miketaylr]

Looks good! Just a few naming/comment issues, then should be good to merge. 👍

[miketaylr]

@moraveyo I'm not sure if this is ready for re-review yet, can you ping when it is?

Thanks!

(no rush, just want to make sure I'm not ignoring your work.)

[moraveyo]

r? @miketaylr
What "bizarre UA strings" can I add to tests, to test the name treatment? (Just can't make it up)

[miketaylr]

What "bizarre UA strings" can I add to tests, to test the name treatment? (Just can't make it up)

You could use some of the bogus stuff in these tests: https://github.com/moraveyo/webcompat.com/blob/eadb82be50b3e6d8e7095c57959725b467b0114e/tests/test_helpers.py#L145-L151, or you could invent something wacky like

"Nozilla/1.0 (TI-83 Graphing Calculator;) CoolBrowser/1.0"

[miketaylr]

@moraveyo can you run pep8 on your changes? There's a few style nits here -- thanks!

pep8 webcompat/helpers.py
webcompat/helpers.py:104:1: W293 blank line contains whitespace
webcompat/helpers.py:105:80: E501 line too long (85 > 79 characters)
webcompat/helpers.py:109:19: E701 multiple statements on one line (colon)
webcompat/helpers.py:111:17: E701 multiple statements on one line (colon)
webcompat/helpers.py:113:17: E701 multiple statements on one line (colon)
webcompat/helpers.py:114:19: E221 multiple spaces before operator
webcompat/helpers.py:119:1: W293 blank line contains whitespace

[moraveyo]

Oops... I first tried pep8 --show-source --show-pep8 . from CONTRIBUTING.md, and it does million of messages, so I thought it's broken.

[moraveyo]

r? @miketaylr
As to "bizarre UA strings", I thought about some string resulting in 'family':'Other' with any version number. After some research I see it's not possible.
So I guess tests already cover things, and it's done.

[miketaylr]

LGTM! We just need to clean up the git history. Maybe 3 commits: 1 for the unified code refactor, one for the new tests, and one for fixing the existing tests? LMK if you get stuck on that!

[moraveyo]

r? @miketaylr
I'm putting the code with new tests in one commit: there already was a commit with both, I don't know how to split it.
Hope I didn't do wrong with Rebase.

[moraveyo]

Looks like I did the mess with rebase

[moraveyo]

r? @miketaylr
Now I hope it's OK.

[miketaylr]

Awesome, thanks! And yeah, splitting git commits is not fun (or easy...). This looks great, thanks for all your work here.

[miketaylr]

(there were some pep8 errors, but I pushed a fixup commit here: e02eccb)

[moraveyo]

Strange, I've already removed some spaces so that pep8 tests/test_helpers.py made no messages for remaining ones.
All those spaces were for visual clearness...

[moraveyo]

I first even tried # nopep8 at the end of each line - didn't work.

[miketaylr]

Hmm, odd that we saw different things. In general, python style doesn't like whitespace for alignment. See https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements (look for "More than one space around an assignment (or other) operator to align it with another. ")