@hroncok commented on this pull request.
Those are my final thoughts. They are not serious, but I thought I'd share them
as suggestions.
> +try:
+ from importlib.metadata import PathDistribution
+except ImportError:
+ from importlib_metadata import PathDistribution
+
+try:
+ from pathlib import Path
+except ImportError:
+ from pathlib2 import Path
+
+
+class Req(Requirement):
+ def __init__(self, requirement_string):
+ super(Req, self).__init__(requirement_string)
+ self.normalized_name = normalize_name(self.name)
+ self.legacy_normalized_name = legacy_normalize_name(self.name)
It's confusing for me to not see the functions defined before this class.
> from warnings import warn
+try:
+ from importlib.metadata import PathDistribution
+except ImportError:
+ from importlib_metadata import PathDistribution
+
+try:
+ from pathlib import Path
+except ImportError:
+ from pathlib2 import Path
+
+
+class Req(Requirement):
This naming is confusing to me. Why not import Requirement as Requirement_ and
call this Requirement?
> + super(Distribution, self).__init__(Path(path))
+ self.name = self.metadata['Name']
+ self.normalized_name = normalize_name(self.name)
+ self.legacy_normalized_name = legacy_normalize_name(self.name)
+ self.requirements = [Req(r) for r in self.requires or []]
+ self.extras = [
+ v for k, v in self.metadata.items() if k == 'Provides-Extra']
+ self.py_version = self._parse_py_version(path)
+
+ def _parse_py_version(self, path):
+ # Try to parse the Python version from the path the metadata
+ # resides at (e.g. /usr/lib/pythonX.Y/site-packages/...)
+ res = re.search(r"/python(?P<pyver>\d+\.\d+)/", path)
+ if res:
+ return res.group('pyver')
+ # If that hasn't worked, attempt to parse it from the directory name
```suggestion
# If that hasn't worked, attempt to parse it from the metadata
directory name
```
> + return chain.from_iterable(
+ [self.requirements_for_extra(extra) for extra in self.extras])
```suggestion
return chain(self.requirements_for_extra(e) for e in self.extras)
```
(untested)
> @@ -143,10 +201,30 @@ def convert(name, operator, version_id):
def normalize_name(name):
"""https://www.python.org/dev/peps/pep-0503/#normalized-names""";
- import re
Why is this no longer lazy? Because the performance impact was negligible?
>
if args.majorver_provides or args.majorver_provides_versions or \
args.majorver_only or args.legacy_provides or args.legacy:
# Get the Python major version
pyver_major = dist.py_version.split('.')[0]
if args.provides:
# If egg/dist metadata says package name is python, we provide
python(abi)
- if dist.key == 'python':
+ if dist.legacy_normalized_name == 'python':
```suggestion
if dist.normalized_name == 'python':
```
This does the same, but reads easier.
> @@ -324,7 +363,7 @@ def normalize_name(name):
if args.requires or (args.recommends and dist.extras):
name = 'python(abi)'
# If egg/dist metadata says package name is python, we don't
add dependency on python(abi)
- if dist.key == 'python':
+ if dist.legacy_normalized_name == 'python':
```suggestion
if dist.normalized_name == 'python':
```
> (lower.endswith('.egg') or
lower.endswith('.egg-info'))):
- # stick them first so any more specific requirement
overrides it
- deps.insert(0, Requirement.parse('setuptools'))
+ groups = set([ep.group for ep in dist.entry_points])
```suggestion
groups = {ep.group for ep in dist.entry_points}
```
Unless we want to support Python 2.6 which I'd rather not (and most likely we
already don't).
> (lower.endswith('.egg') or
lower.endswith('.egg-info'))):
- # stick them first so any more specific requirement
overrides it
- deps.insert(0, Requirement.parse('setuptools'))
+ groups = set([ep.group for ep in dist.entry_points])
+ if set(["console_scripts", "gui_scripts"]) & groups:
```suggestion
if {"console_scripts", "gui_scripts"} & groups:
```
> - py_deps[name] = []
- spec = ('==', spec[1])
- if spec not in py_deps[name]:
- py_deps[name].append(spec)
-
- names = list(py_deps.keys())
- names.sort()
- for name in names:
+ for dep in dist.requirements + dist.extra_requirements:
+ for spec in dep.specifier:
+ if spec.operator == '!=':
+ if dep.legacy_normalized_name not in py_deps:
+ py_deps[dep.legacy_normalized_name] = []
+ spec = ('==', spec.version)
+ if spec not in py_deps[dep.legacy_normalized_name]:
+
py_deps[dep.legacy_normalized_name].append(spec)
This is mostly unused code now, so not sure if ti works, but ok.
> - if spec not in py_deps[name]:
- py_deps[name].append(spec)
-
- names = list(py_deps.keys())
- names.sort()
- for name in names:
+ for dep in dist.requirements + dist.extra_requirements:
+ for spec in dep.specifier:
+ if spec.operator == '!=':
+ if dep.legacy_normalized_name not in py_deps:
+ py_deps[dep.legacy_normalized_name] = []
+ spec = ('==', spec.version)
+ if spec not in py_deps[dep.legacy_normalized_name]:
+
py_deps[dep.legacy_normalized_name].append(spec)
+
+ for name in sorted(py_deps.keys()):
```suggestion
for name in sorted(py_deps):
```
> +from packaging.requirements import Requirement
+from packaging.version import parse
Let's keep the 3rd party packages import at the end?
> @@ -142,9 +210,23 @@ def convert(name, operator, version_id):
def normalize_name(name):
resolved
--
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-513672311_______________________________________________
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
