@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_______________________________________________
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
