[webkit-reviews] review granted: [Bug 179426] webkitpy: Unify version parsing code : [Attachment 326411] 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 179426: webkitpy: Unify version parsing code
https://bugs.webkit.org/show_bug.cgi?id=179426

Attachment 326411: Patch

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




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

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

r=me

You don't have to make any of the suggested changes before landing, but at
least consider:
- Tightening up __len__ and parsing of versions with more than 3 integers
(maybe throw an exception due to "loss" of data)?
- Add tests for __len__ (whether or not the above is done) and __setitem__ to
make sure they work.
- Consider using 'tiny' instead of 'build'.  (The word 'build' is very
over-loaded, and 'build version' means something different than the 3rd integer
in a dotted version string at Apple.)

> Tools/ChangeLog:20
> +	   (PlatformInfo.__init__): Convert mac version string to version
object and
> +	   use _win_version instead of _win_version_tuple.
> +	   (PlatformInfo.xcode_sdk_version): Convert SDK version string to
Version object
> +	   before returning it.
> +	   (PlatformInfo.xcode_version): Return version object instead of
version string.
> +	   (PlatformInfo._determine_mac_version): Accept version object instead
of string,
> +	   eliminate parsing.
> +	   (PlatformInfo._determine_win_version): Accept version object instead
of tuple.

Nit:  Not using capital "Version object" consistently.	Doesn't have to be
fixed for landing.

> Tools/Scripts/webkitpy/common/version.py:29
> +	   self.major = 0
> +	   self.minor = 0
> +	   self.build = 0

In Source/JavaScriptCore/Configurations/Version.xcconfig,the following names
are used for each position:

MAJOR_VERSION = 605;
MINOR_VERSION = 1;
TINY_VERSION = 13;
MICRO_VERSION = 0;
NANO_VERSION = 0;

Not sure this class needs to support 5 dotted versions, but you might consider
renaming 'build' to 'tiny' for consistency.

(I actually don't know what the history of those names is, though.)

Not required to land the patch.

> Tools/Scripts/webkitpy/common/version.py:43
> +	   raise ValueError('Version expected to be int, str, tuple or list of
ints')

Nit: Spell out "ints" as "integers".

> Tools/Scripts/webkitpy/common/version.py:46
> +    def __len__(self):
> +	   return 3

This doesn't return a correct value if one of the for loops in the __init__
method is used as it will set keys for '3' and '4' (or more).

May not matter currently, but it should do the right thing if we support
passing in "1.2.3.4" or "1.2.3.4.5".

Hmm...then again, we raise an exception in __getitem__ and __setitem__ if the
key is larger than '2'.  Not sure what you want to do here.

> Tools/Scripts/webkitpy/common/version.py:98
> +    def __str__(self):
> +	   result = '{}'.format(self.major)
> +	   if self.minor or self.build:
> +	       result += '.{}'.format(self.minor)
> +	   if self.build:
> +	       result += '.{}'.format(self.build)
> +	   return result

If we support more than 3 dotted numbers (see above comments), may want to make
this more generic?

> Tools/Scripts/webkitpy/common/version_unittest.py:28
> +class VersionTestCase(unittest.TestCase):

One of these tests should verify __len__.

You should also have a test for using __setitem__ with both numbers (0, 1, 2)
and strings (major, minor, build/tiny).

> Tools/Scripts/webkitpy/common/system/platforminfo.py:61
>	   if self.os_name.startswith('mac'):
> -	       self.os_version =
self._determine_mac_version(platform_module.mac_ver()[0])
> +	       self.os_version =
self._determine_mac_version(Version(platform_module.mac_ver()[0]))
>	   if self.os_name.startswith('win'):
> -	       self.os_version =
self._determine_win_version(self._win_version_tuple(sys_module))
> +	       self.os_version =
self._determine_win_version(self._win_version(sys_module))

Kind of weird how platform_module.mac_ver() doesn't return a Version object
here, but this doesn't block the patch from landing.

Maybe too much to change for this patch (which is already pretty large)?

> Tools/Scripts/webkitpy/port/ios_simulator.py:191
>      def use_multiple_simulator_apps(self):
> -	   return int(self.host.platform.xcode_version().split('.')[0]) < 9
> +	   return int(self.host.platform.xcode_version()[0]) < 9

After reading the patch up to this point, I find that using major/minor/build
is probably more readable that 0/1/2.

Not necessary to fix before landing, though.