Replace pkg_resources.parse_version with packaging.version.parse #693

ijnek · 2023-02-02T03:11:44Z

Resolves: #678.

Switch to packaging since use of pkg_resources is discouraged. Related: pypa/setuptools#2497

Also:

Uses regex to do version matching, because packaging's version parsing is a lot more strict about version numbers.
Also, removes a check for whether a mac is Mountain Lion or greater and just changes it to a check for whether it is a mac.

Signed-off-by: ijnek <kenjibrameld@gmail.com>

bloom/logging.py

Co-authored-by: Jochen Sprickerhof <github@jochen.sprickerhof.de>

nuclearsandwich

Thanks @ijnek for this PR! and @jspricke for an earlier review!

While I'm not thrilled about the introduction of an additional dependency, it is one that is part of the extended suite of python packaging related dependencies and is widely packaged and available so it's not overly concerning.

This switch also resolves CI which is currently failing due to an apparent behavior change in how the deprecated pkg_resources function handles empty strings.

I have a couple of non-blocking suggestions but found one regex that needs feedback as I think it's incorrect as-is.

I also raised a question or two of general maintenance surfaced by looking at some changed areas which do not block merging this PR.

Once the regex is looked at again this should be quite close to ready.

nuclearsandwich · 2023-03-04T00:27:48Z

bloom/commands/git/import_upstream.py

@@ -39,7 +39,7 @@
 import tarfile
 import tempfile

-from pkg_resources import parse_version
+from packaging.version import parse


Being a bit of a python outsider, I've always disliked that a function's utility is sometimes obfuscated when the functions are imported outside their module.
Does using the alias-on-import feature make sense so that parse doesn't lose its context.
If using the alias-on-import feature is not seen as idiomatic then I could live with it as is, but I think it improves local legibility.

Suggested change

from packaging.version import parse

from packaging.version import parse as parse_version

Another alternative that is used in the other files is to import the entire version module which would afford the same context as the function would be invoked as version.parse().

Suggested change

from packaging.version import parse

from packaging import version

I would say the function name should just be something more specific than parse, but I think your first suggestion is just fine for now.

It looks like I've avoided using from packaging import version, because the parameter for the version_check method was also version and there was a conflict.

But I can go with parse_version.

bloom/commands/git/import_upstream.py

bloom/generators/release.py

nuclearsandwich · 2023-03-04T00:35:55Z

bloom/git.py

@@ -686,7 +687,9 @@ def get_last_tag_by_version(directory=None):
    versions = []
    for line in output.splitlines():
        tags.append(line.strip())
-        versions.append(parse_version(line.strip()))
+        ver = re.match("[0-9]+.[0-9]+.[0-9]+", line)


Suggested change

ver = re.match("[0-9]+.[0-9]+.[0-9]+", line)

ver = re.match(r"[0-9]+\.[0-9]+\.[0-9]+", line)

Since dots are regex metacharacters they need to be escaped for an exact match. Or is the intent that any character could separate these digits?

This is likely what was intended.

Resolved in cd20317

bloom/logging.py

Signed-off-by: ijnek <kenjibrameld@gmail.com>

…n" is used as variables everywhere Signed-off-by: ijnek <kenjibrameld@gmail.com>

ijnek · 2023-03-06T04:28:03Z

Across all files, I've gone with from packaging.version import parse as parse_version, because the parse isn't descriptive enough, and version.parse causes a lot of conflicts with local variables and parameters named version.

nuclearsandwich

Thanks for iterating @ijnek and for this excellent improvement. Among other things I believe this addresses the issue that is affecting CI for all Bloom PRs right now.

This was introduced in #693 and I realized that it needed the stdeb dependency while I was on a walk but forgot to add that before re-reviewing and merging that PR. This package is available for Python 2 in [Buster](https://packages.debian.org/buster/python3-packaging) and [Bionic](https://packages.ubuntu.com/bionic/python-packaging). As well as for Python 3 in [Buster](https://packages.debian.org/buster/python3-packaging) and [Bionic](https://packages.ubuntu.com/bionic/python3-packaging) and later distributions.

ijnek force-pushed the ijnek-#678 branch from 2fd012a to 1df61b7 Compare February 2, 2023 03:24

replace parse_version with packaging.version.parse

8a1e957

Signed-off-by: ijnek <kenjibrameld@gmail.com>

ijnek force-pushed the ijnek-#678 branch from 1df61b7 to 8a1e957 Compare February 2, 2023 03:28

ijnek added 3 commits February 2, 2023 03:36

lstrip upstream/

ff1483b

Signed-off-by: ijnek <kenjibrameld@gmail.com>

prevent conflict of "version" word

57f13b1

Signed-off-by: ijnek <kenjibrameld@gmail.com>

use regex to get version number

c4c1049

Signed-off-by: ijnek <kenjibrameld@gmail.com>

ijnek force-pushed the ijnek-#678 branch from b140708 to c4c1049 Compare February 2, 2023 03:59

ijnek changed the title ~~Replace parse_version with packaging.version.parse~~ Replace pkg_resources.parse_version with packaging.version.parse Feb 2, 2023

ijnek marked this pull request as ready for review February 2, 2023 04:07

jspricke approved these changes Feb 2, 2023

View reviewed changes

jspricke reviewed Feb 8, 2023

View reviewed changes

bloom/logging.py Outdated Show resolved Hide resolved

Update bloom/logging.py

a228a51

Co-authored-by: Jochen Sprickerhof <github@jochen.sprickerhof.de>

nuclearsandwich mentioned this pull request Mar 4, 2023

CI failing on master #696

Open

nuclearsandwich requested changes Mar 4, 2023

View reviewed changes

ijnek added 2 commits March 5, 2023 22:02

import version.parse as parse_version

d8e4d15

Signed-off-by: ijnek <kenjibrameld@gmail.com>

change to use parse_version instead of version.parse, because "versio…

cd20317

…n" is used as variables everywhere Signed-off-by: ijnek <kenjibrameld@gmail.com>

nuclearsandwich approved these changes Mar 10, 2023

View reviewed changes

nuclearsandwich merged commit b34dad4 into ros-infrastructure:master Mar 10, 2023

ijnek deleted the ijnek-#678 branch March 10, 2023 01:46

nuclearsandwich mentioned this pull request Mar 10, 2023

Add debian dependency to stdeb.cfg for new packaging dependency. #698

Merged

tfoote mentioned this pull request Jan 17, 2024

BUG: bloom-release fails to run on humble because of mac_ver #722

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace pkg_resources.parse_version with packaging.version.parse #693

Replace pkg_resources.parse_version with packaging.version.parse #693

ijnek commented Feb 2, 2023 •

edited

Loading

nuclearsandwich left a comment

nuclearsandwich Mar 4, 2023

nuclearsandwich Mar 4, 2023

wjwwood Mar 4, 2023

ijnek Mar 6, 2023

nuclearsandwich Mar 4, 2023

wjwwood Mar 4, 2023

nuclearsandwich Mar 10, 2023

ijnek commented Mar 6, 2023

nuclearsandwich left a comment

	from packaging.version import parse
	from packaging.version import parse as parse_version

	from packaging.version import parse
	from packaging import version

	ver = re.match("[0-9]+.[0-9]+.[0-9]+", line)
	ver = re.match(r"[0-9]+\.[0-9]+\.[0-9]+", line)

Replace pkg_resources.parse_version with packaging.version.parse #693

Replace pkg_resources.parse_version with packaging.version.parse #693

Conversation

ijnek commented Feb 2, 2023 • edited Loading

nuclearsandwich left a comment

Choose a reason for hiding this comment

nuclearsandwich Mar 4, 2023

Choose a reason for hiding this comment

nuclearsandwich Mar 4, 2023

Choose a reason for hiding this comment

wjwwood Mar 4, 2023

Choose a reason for hiding this comment

ijnek Mar 6, 2023

Choose a reason for hiding this comment

nuclearsandwich Mar 4, 2023

Choose a reason for hiding this comment

wjwwood Mar 4, 2023

Choose a reason for hiding this comment

nuclearsandwich Mar 10, 2023

Choose a reason for hiding this comment

ijnek commented Mar 6, 2023

nuclearsandwich left a comment

Choose a reason for hiding this comment

ijnek commented Feb 2, 2023 •

edited

Loading