[Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)

[torsava]

@torsava requested changes on this pull request.

Hi @s-t-e-v-e-n-k ,
first of all, I'm really sorry the review took so long :(

I've pointed out some issues in the comments, mostly in the less-or-not-used parts of the code.

The PR as a whole looks fantastic, great piece of engineering. And all our tests are passing. Thanks!

> +        if isdir(path):
+            path_item = dirname(path)
+        else:
+            path_item = path

Why not just `path_item = path` (or just use `path` below instead)?

> -                        dep_normalized_name = dep.key
+                        dep_normalized_name = dep.name.lower().replace('_', '-')
 
                     if args.legacy:
-                        name = 'pythonegg({})({})'.format(pyver_major, dep.key)
+                        name = 'pythonegg({})({})'.format(pyver_major, dep.name.lower())

I've never used the legacy mode, I'm not even sure if anyone uses it. But for consistency: Why is `dep.key` being replaced by slightly different expressions in these two places?

> -                                print('Conflicts:\t{} {} {}'.format(dep.key, '==', spec[1]))
+                    for dep in dist.requirements_for_extra(extra):
+                        for spec in dep.specifier:
+                            if spec.operator == '!=':
+                                print('Conflicts:\t{} {} {}'.format(dep.name, '==', spec.version))
                             else:
-                                print('Requires:\t{} {} {}'.format(dep.key, spec[0], spec[1]))
+                                print('Requires:\t{} {} {}'.format(dep.name, spec.operator, spec.version))

Another two places where `dep.key` is being replaced by yet different expression. Maybe it would be worth creating the `key` property for `Distribution` and unifying it.

> -                for dep in dist.requires(extras=dist.extras):
-                    name = dep.key
-                    for spec in dep.specs:
-                        if spec[0] == '!=':
+                for dep in dist.requirements + dist.extra_requirements:
+                    for spec in dep.specifier:
+                        if spec.operator == '!=':
                             if name not in py_deps:
-                                py_deps[name] = []
-                            spec = ('==', spec[1])
+                                py_deps[dep.name] = []
+                            spec = ('==', spec.version)
                             if spec not in py_deps[name]:
-                                py_deps[name].append(spec)
+                                py_deps[dep.name].append(spec)

`name` is not initialized here, and `dep.name` is another replacement for `dep.key` which is not consistent with the others.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1317#pullrequestreview-493449129
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20200922/a2194b3e/attachment.html>