yeongbin-jo / python-chromedriver-autoinstaller

The chromedriver auto installer for distribution.
MIT License
235 stars 77 forks source link

version comparison issue #56

Closed jinhee-kim closed 9 months ago

jinhee-kim commented 1 year ago

Using strings when comparing versions can cause problems.

Example:

'100.0.4845.0' > '99.0.4844.94`  # False
'99.0.4844.94' > '115'  # True

Code: https://github.com/yeongbin-jo/python-chromedriver-autoinstaller/blob/e66bfceeb504b5d6f7f0a380c4551f6ad1fa6a0f/chromedriver_autoinstaller/utils.py#L59 https://github.com/yeongbin-jo/python-chromedriver-autoinstaller/blob/e66bfceeb504b5d6f7f0a380c4551f6ad1fa6a0f/chromedriver_autoinstaller/utils.py#L62 https://github.com/yeongbin-jo/python-chromedriver-autoinstaller/blob/e66bfceeb504b5d6f7f0a380c4551f6ad1fa6a0f/chromedriver_autoinstaller/utils.py#L68 https://github.com/yeongbin-jo/python-chromedriver-autoinstaller/blob/e66bfceeb504b5d6f7f0a380c4551f6ad1fa6a0f/chromedriver_autoinstaller/utils.py#L95 https://github.com/yeongbin-jo/python-chromedriver-autoinstaller/blob/e66bfceeb504b5d6f7f0a380c4551f6ad1fa6a0f/chromedriver_autoinstaller/utils.py#L221

shawnCaza commented 1 year ago

"revision" instead of "version" seems like a better comparison point to me when using the known-good-versions.json

matiasbertani commented 1 year ago

This can be fixed using version from packaging library. I think it is cleaner and prevents us to think about this kind of issue.

from packaging import version

 v1 = version.parse('100.0.4845.0')
 v2 = version.parse('99.0.4844.94')
 v3 = version.parse('115')

v1 > v2  # True
v2 > v3  # False

Shall I start a new PR with this change?

shawnCaza commented 1 year ago

@matiasbertani that's an interesting option, but with the use of the latest-versions-per-milestone-with-downloads.json end point that's been added to #54 or #57 we don't need to think about versioning. That end point just gives us the most recent version per milestone.

shawnCaza commented 1 year ago

Using the latest version per milestone json eliminated the need for comparison when getting the latest driver.

To determine the correct mac platform/architexture the major version is all that's require for >=115. However there is one line where I think @matiasbertani packaging library suggestion may improve accuracy:

elif chrome_version is not None and version.parse(chrome_version) <= version.parse("106.0.5249.21"):

I've incorporated the version check changes into #57