[webkit-reviews] review granted: [Bug 179621] webkitpy: Better name-version mapping : [Attachment 327068] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 179621: webkitpy: Better name-version mapping
https://bugs.webkit.org/show_bug.cgi?id=179621

Attachment 327068: Patch

https://bugs.webkit.org/attachment.cgi?id=327068&action=review




--- Comment #7 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 327068
  --> https://bugs.webkit.org/attachment.cgi?id=327068
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327068&action=review

r=me with a few comments, but Dan Bates expressed a concern, so please sync up
with him before landing.

My opinion is that it's an improvement to have all this logic in one place (and
tested), and it will make it easy to add new versions in the future.

> Tools/Scripts/webkitpy/common/version.py:103
> +    # 11.2 is contained in 11, but 11 is not contained in 11.2
> +    def contained_in(self, version):
> +	   match_flag = False
> +	   for i in xrange(len(self)):
> +	       if self[i] != version[i]:
> +		   match_flag = True
> +	       if match_flag and version[i] != 0:
> +		   return False
> +	   return True

I'm having a hard time with the name 'match_flag'.  I'm sure you wanted to
avoid double-negatives, but maybe a name like this might read better?

    last_number_did_not_match
    did_not_match

Or reverse the logic and use 'did_match' instead.

It would also be nice to use something other than "number", but not sure what
to call each set of digits between periods.  (Probably not "sub_version",
though. :)  Maybe "intermediate_version", although that's really long.

I guess that's probably why you chose 'match_flag'.

Not necessary to fix before landing, but something to consider.

> Tools/Scripts/webkitpy/common/version_name_map.py:77
> +	       'linux': {
> +		   'Lucid': Version('10.4'),
> +		   'Maverick': Version('10.10'),
> +		   'Natty': Version('11.4'),
> +		   'Precise': Version('12.4'),
> +		   'Quantal': Version('12.10'),
> +		   'Raring': Version('13.4'),
> +		   'Saucy': Version('13.10'),
> +		   'Utopic': Version('14.10'),
> +		   'Vivid': Version('15.04'),
> +		   'Wily': Version('15.10'),
> +		   'Yakkety': Version('16.10'),
> +		   'Bionic': Version('18.04'),
> +	       },

Are these all Ubuntu names?  Haven't heard of them before.

> Tools/Scripts/webkitpy/common/version_name_map.py:112
> +	   if ' ' in name:
> +	       try:
> +		   name = '{}{}'.format(name.split(' ')[0],
Version(''.join(name.split(' ')[1:])).major)

I think this will fail for "El Capitan 10.11".

Probably better to split on space, then pop off the last item for Version
parsing, then join the remaining ones with spaces for name parsing.

Oh, but maybe that will never be used because we don't use version numbers with
macOS names, though?  If so, no changes needed here.

> Tools/Scripts/webkitpy/common/version_name_map_unittest.py:42
> +    def test_mac_version_by_name(self):
> +	   map = VersionNameMap()
> +	   self.assertEqual(map.from_name('Sierra'), ('mac', Version('10.12')))
> +	   self.assertEqual(map.from_name('sierra'), ('mac', Version('10.12')))
> +	   self.assertEqual(map.from_name('El Capitan'), ('mac',
Version('10.11')))
> +	   self.assertEqual(map.from_name('elcapitan'), ('mac',
Version('10.11')))

Maybe add unit test:

    self.assertEqual(map.from_name('el Capitan'), ('mac', Version('10.11')))

Maybe reverse arguments here from (actual, expected) to (expected, actual)?

> Tools/Scripts/webkitpy/common/version_name_map_unittest.py:48
> +    def test_ios_version_by_name(self):
> +	   map = VersionNameMap()
> +	   self.assertEqual(map.from_name('iOS 11'), ('ios', Version(11)))
> +	   self.assertEqual(map.from_name('ios11'), ('ios', Version(11)))
> +	   self.assertEqual(map.from_name('iOS 11.2'), ('ios', Version(11)))

Should these work?

    self.assertEqual(map.from_name('ios11.2'), ('ios', Version(11)))
    self.assertEqual(map.from_name('iOS11.2'), ('ios', Version(11)))

Maybe reverse arguments here from (actual, expected) to (expected, actual)?

> Tools/Scripts/webkitpy/common/version_unittest.py:131
> +    def test_contained_in(self):
> +	   self.assertTrue(Version('11.1').contained_in(Version('11')))
> +	   self.assertTrue(Version('11.1.2').contained_in(Version('11.1')))
> +	   self.assertFalse(Version('11').contained_in(Version('11.1')))

Maybe add more test cases:

self.assertTrue(Version('11').contained_in(Version('11.1.2')))
self.assertTrue(Version('11.1').contained_in(Version('11.1.2')))
self.assertTrue(Version('11').contained_in(Version('11')))
self.assertTrue(Version('11.1').contained_in(Version('11.1')))
self.assertTrue(Version('11.1.2').contained_in(Version('11.1.2')))
self.assertTrue(Version('11').contained_in(Version('11.0')))
self.assertTrue(Version('11.0').contained_in(Version('11')))
self.assertTrue(Version('11').contained_in(Version('11.0.0')))
self.assertTrue(Version('11.0.0').contained_in(Version('11')))
self.assertTrue(Version('11.1').contained_in(Version('11.1.0')))
self.assertTrue(Version('11.1.0').contained_in(Version('11.1')))

> Tools/Scripts/webkitpy/common/system/platforminfo.py:65
> +	   else:
> +	       # Linux and iOS both return conforming version strings.
> +	       self.os_version =
VersionNameMap.map(self).to_name(Version(platform_module.release()),
table='public')

Replace the comment with:

	assert self.os_name == 'ios' or self.os_name == 'linux'