Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
🎉 🥳 🎊 -- 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#issuecomment-760690153___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
Merged #1317 into master. -- 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#event-4209603221___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
Sorry, this has gone on so long that I totally missed that it's finally done :sweat_smile: -- 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#issuecomment-760689436___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@pmatilai Please? :crying_cat_face: -- 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#issuecomment-760231299___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
So, who has the power to merge this? @ffesti ? -- 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#issuecomment-748928961___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@Conan-Kudo approved this pull request. -- 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-534495168___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
> Oh, I've been alerted by @torsava that my suggestions have been addressed. > Nice, thanks! The code looks good to me. If the external tests still pass, > let's ship it? > > We can probably test this in Fedora before merging if we want more testing > (but this is not necessary). > > Thank you @s-t-e-v-e-n-k! I'd prefer to merge it here, we've delayed the PR long enough :) We'll test it in Fedora in any case, and we can address possible issues later. Thank you @s-t-e-v-e-n-k for having patience with us :) -- 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#issuecomment-724772529___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
Oh, I've been alerted by @torsava that my suggestions have been addressed. Nice, thanks! The code looks good to me. If the external tests still pass, let's ship it? We can probably test this in Fedora before merging if we want more testing (but this is not necessary). Thank you @s-t-e-v-e-n-k! -- 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#issuecomment-724646250___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
This pull request **fixes 1 alert** when merging 1eeb6b66e2933881f78a7d870bab3f2ee29bf216 into 98b71f7a92e8e9d902e2bfcd6c30af84dd80289b - [view on LGTM.com](https://lgtm.com/projects/g/rpm-software-management/rpm/rev/pr-88d3a1c4115318fb2c38b78151955c51b3357bbd) **fixed alerts:** * 1 for Module is imported more than once -- 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#issuecomment-721487004___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request. > +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) With a change this big, I wouldn't worry about that. -- 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#discussion_r515074212___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@s-t-e-v-e-n-k commented on this pull request. > @@ -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 Indeed -- 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#discussion_r514807140___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@s-t-e-v-e-n-k commented on this pull request. > +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) I'm trying to keep the diff small, but I can move the function definition up if you wish. -- 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#discussion_r514806181___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@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\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:
+
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava approved this pull request. Looks good to me! -- 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-508091467___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
This pull request **fixes 1 alert** when merging 62f43dbb7bffc9f978d1c3427a94b8cf7e2bfd52 into b766e63de30d2e2ee103d132e35a13e82a62cd27 - [view on LGTM.com](https://lgtm.com/projects/g/rpm-software-management/rpm/rev/pr-12f4bd115540f9a4fa1bf4c75bd4314b1d88a7bd) **fixed alerts:** * 1 for Module is imported more than once -- 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#issuecomment-708146074___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request. > @@ -142,9 +210,23 @@ def convert(name, operator, version_id): def normalize_name(name): @s-t-e-v-e-n-k Thinking more on this, since we have to keep this function, and since the class/static methods you currently use are used inside 2 different classes anyway, how about implementing the functionality as normal global functions? That way they could be used outside this script easily, and we would not need this wrapper. -- 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#discussion_r503744048___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
This pull request **fixes 1 alert** when merging 62088ae736b4eac30758209d9537fa1089f6e02c into 6163c6cfd99f1dbafd89962598729e7a81932311 - [view on LGTM.com](https://lgtm.com/projects/g/rpm-software-management/rpm/rev/pr-44669caf5b3c6cb2c541d0f66685a0972b6ae983) **fixed alerts:** * 1 for Module is imported more than once -- 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#issuecomment-707563659___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok commented on this pull request. > @@ -142,9 +210,23 @@ def convert(name, operator, version_id): def normalize_name(name): Happy to adapt that script when we integrate this. -- 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#discussion_r503740667___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request. > @@ -142,9 +210,23 @@ def convert(name, operator, version_id): def normalize_name(name): Ah, right. In that case, yeah, a comment would be great. -- 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#discussion_r503734008___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@s-t-e-v-e-n-k commented on this pull request. > @@ -142,9 +210,23 @@ def convert(name, operator, version_id): def normalize_name(name): It can't, and I wish it could be -- https://src.fedoraproject.org/rpms/python-rpm-generators/blob/master/f/pythonbundles.py#_41 -- 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#discussion_r503587276___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@s-t-e-v-e-n-k commented on this pull request. > +@classmethod +def normalize_name(klass, name): +"""https://www.python.org/dev/peps/pep-0503/#normalized-names"""; +return re.sub(r'[-_.]+', '-', name).lower() + +@classmethod +def legacy_normalize_name(klass, name): +"""Like pkg_resources Distribution.key property""" +return re.sub(r'[-_]+', '-', name).lower() Ah, good point! staticmethod makes much more sense. -- 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#discussion_r503586786___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok commented on this pull request. > +@classmethod +def normalize_name(klass, name): +"""https://www.python.org/dev/peps/pep-0503/#normalized-names"""; +return re.sub(r'[-_.]+', '-', name).lower() + +@classmethod +def legacy_normalize_name(klass, name): +"""Like pkg_resources Distribution.key property""" +return re.sub(r'[-_]+', '-', name).lower() I wonder why it is a classmethod and not a staticmethod (when it does not use the "kalss" attribute at all). I would make it a property, but initialized value is fine as well. -- 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#discussion_r503325494___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request. > +@classmethod +def normalize_name(klass, name): +"""https://www.python.org/dev/peps/pep-0503/#normalized-names"""; +return re.sub(r'[-_.]+', '-', name).lower() + +@classmethod +def legacy_normalize_name(klass, name): +"""Like pkg_resources Distribution.key property""" +return re.sub(r'[-_]+', '-', name).lower() > I wonder why it is a classmethod and not a staticmethod (when it does not use > the "kalss" attribute at all). I would make it a property, but initialized > value is fine as well. It's also used outside of this class, so it can't be a property. But a static method/function makes sense. -- 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#discussion_r503360841___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request. Besides the two mostly cosmetic comments, I think it looks good! > @@ -142,9 +210,23 @@ def convert(name, operator, version_id): def normalize_name(name): This function is now completely unused and can be removed. > +@classmethod +def normalize_name(klass, name): +"""https://www.python.org/dev/peps/pep-0503/#normalized-names"""; +return re.sub(r'[-_.]+', '-', name).lower() + +@classmethod +def legacy_normalize_name(klass, name): +"""Like pkg_resources Distribution.key property""" +return re.sub(r'[-_]+', '-', name).lower() Nitpick: I'm not excited about the `klass` name instead of the standard `cls`. -- 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-506555419___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
This pull request **fixes 1 alert** when merging 29f742a6b45b0d33463b963347cd083a1ebb2a71 into 257077a60c719bf98915c17fa71dc6c68fb0c3e9 - [view on LGTM.com](https://lgtm.com/projects/g/rpm-software-management/rpm/rev/pr-f2e0588b56e5d2ad96ee5444c393d5575e5698d0) **fixed alerts:** * 1 for Module is imported more than once -- 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#issuecomment-701921756___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@s-t-e-v-e-n-k pushed 1 commit. 29f742a6b45b0d33463b963347cd083a1ebb2a71 pythondistdeps: Switch to importlib.metadata -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1317/files/20ad7b9b4845386ddb54ce2a88465e00d992fd0c..29f742a6b45b0d33463b963347cd083a1ebb2a71 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
And I agree with unifying the two ways to get a normalized name.
--
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#discussion_r496527810___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
@s-t-e-v-e-n-k I wasn't against ceasing to use the `key` property, I was
against passing different values in it's place. The name of the
property/function is immaterial.
--
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#discussion_r496524935___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@s-t-e-v-e-n-k commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
I'm sorry, I misread that completely and conflated it with the argument
parsing, my apologies.
This would also require massive changes everywhere to stop using the key
property, which @torsava has seemed against -- but I'm happy to do so.
Secondly, this doesn't solve it for Distribution, so anything we do for
requirements we should also do for the Distribution class -- my current idea is
a classmethod on Req that we call there and in Distribution. And a consequence
the `normalize_name()` function gets removed as well.
`re.sub('[^A-Za-z0-9.]+', '-', self.name).lower()` is what Distribution current
does, which is what `pkg_resources.safe_name()` does.
--
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#discussion_r496409925___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
I'm also confused as to what that means..?
--
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#discussion_r494836924___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
massive layering violation?
--
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#discussion_r494314287___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@s-t-e-v-e-n-k commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
Which is a massive layering violation. I'll have a think on this.
--
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#discussion_r494312871___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
Instead, I'd create two properties here:
```python
class Req(Requirement):
@property
def legacy_normalized_name(self):
"""Like pkg_resources key property"""
if not self._legacy_normalized_name:
self._legacy_normalized_name = re.sub(r'[-_]+', '-',
self.name).lower()
 return self._legacy_normalized_name
@property
 def normalized_name(self):
"""PEP 503 normalized name"""
if not self._normalized_name:
self._normalized_name = re.sub(r'[-_.]+', '-', self.name).lower()
 return self._normalized_name
```
"key" is not very descriptive.
--
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#discussion_r494237504___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
Ah, I see. Yeah, that should be fixed as well. @s-t-e-v-e-n-k please take a
look.
--
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#discussion_r494236346___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
That really depends on `self.name` value. I don't see that from the code here.
In pkg_resources, key is based on `self.project_name` and that [is set
to](https://github.com/pypa/setuptools/blob/73e379cc55ac1e9ec63c4ac30b75ecc82418f513/pkg_resources/__init__.py#L2564)
`safe_name(project_name)`.
In packaging, I believe `self.name` is not pre-processed. See:
```pycon
>>> import pkg_resources
>>> pkg_resources.safe_name("Tree-_-FALLING.Down").lower()
'tree-falling.down'
>>> from packaging.requirements import Requirement
>>> Requirement("Tree-_-FALLING.Down").name.lower().replace('_', '-')
'tree---falling.down'
```
--
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#discussion_r494234076___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
To clarify a possible confusion, I think `key` always returned the same thing.
But this script had two modes, in legacy mode it used the `key` value, and in
pep503 mode it used the `normalize_name` function.
So yes, there are two different methods to convert the name, but that is
handled by this script, not by `Requirement.key`.
--
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#discussion_r494230415___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
Oh, how come? You think the [`Requirement.key`
property](https://setuptools.readthedocs.io/en/latest/pkg_resources.html) gave
different results in different situations?
--
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#discussion_r494227693___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok commented on this pull request.
> 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):
+@property
+def key(self):
+return self.name.lower().replace('_', '-')
@torsava This is the implementation of key. I don't believe it matches the
original key value in all cases.
--
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-495454542___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request. > if normalized_names_require_pep503: -dep_normalized_name = normalize_name(dep.project_name) +dep_normalized_name = normalize_name(dep.name) else: dep_normalized_name = dep.key > Be careful with manual replaces for normalized names. There are two regexes > that this code supported before. The PEP 503 regex and the regex imported > from setuptools (described in here). I wasn't exactly sure what you meant by this comment, so I asked if the new code looks good to you to be sure. -- 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#discussion_r494224179___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok commented on this pull request. > if normalized_names_require_pep503: -dep_normalized_name = normalize_name(dep.project_name) +dep_normalized_name = normalize_name(dep.name) else: dep_normalized_name = dep.key In that case, I don't get the question :) -- 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#discussion_r494210959___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request. > if normalized_names_require_pep503: -dep_normalized_name = normalize_name(dep.project_name) +dep_normalized_name = normalize_name(dep.name) else: dep_normalized_name = dep.key @hroncok But the code continues to preserve both the legacy and pep503 conventions. So I'm not sure I understand what you mean. -- 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-495425833___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok commented on this pull request.
> else:
-dep_normalized_name = dep.key
+dep_normalized_name = dep.key.replace('_', '-')
The code allows both naming conventions. We need to preserve that. ideally, we
would have our own regexes for that to avoid changes based on 3rd party
changes. We have `normalize_name()` function. What about having
`normalize_name(kind="legacy"/"pep503")`?
--
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#discussion_r494174964___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request.
> else:
-dep_normalized_name = dep.key
+dep_normalized_name = dep.key.replace('_', '-')
The newest version looks good to me, and tests are passing.
The [documentation for
pkg_resources](https://setuptools.readthedocs.io/en/latest/pkg_resources.html)
says about `key`: An all-lowercase version of the project_name, useful for
comparison or indexing.
>From there you have to look up `project_name` and then `safe_name` function
>which generates it. So if we go by documentation, dots are not allowed in
>`key`. However, if you actually test it, they are there.
What do you think, @hroncok ?
--
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#discussion_r494166167___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
This pull request **fixes 1 alert** when merging 20ad7b9b4845386ddb54ce2a88465e00d992fd0c into 4cbdd7c94071f52dfe64466df80e859193d89d89 - [view on LGTM.com](https://lgtm.com/projects/g/rpm-software-management/rpm/rev/pr-18cffc102335788681d5ee55cf65821e09632595) **fixed alerts:** * 1 for Module is imported more than once -- 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#issuecomment-698141863___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@s-t-e-v-e-n-k pushed 1 commit. 20ad7b9b4845386ddb54ce2a88465e00d992fd0c pythondistdeps: Switch to importlib.metadata -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1317/files/8a5494c5d22b620a071b73d8b3dc31f495a7cdcd..20ad7b9b4845386ddb54ce2a88465e00d992fd0c ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok commented on this pull request.
> else:
-dep_normalized_name = dep.key
+dep_normalized_name = dep.key.replace('_', '-')
Be careful with manual replaces for normalized names. There are two regexes
that this code supported before. The PEP 503 regex and the regex imported from
setuptools ([described in
here](https://github.com/pypa/setuptools/issues/1597)).
--
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#discussion_r493384880___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request.
> -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))
@s-t-e-v-e-n-k Here you still use `dep.name` as a replacement for `dep.key`, is
there a reason?
--
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#discussion_r493363804___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request.
> else:
-dep_normalized_name = dep.key
+dep_normalized_name = dep.key.replace('_', '-')
In my testing, the previous setuptools version already has underscores replaced
in the `key` property, so I would suggest doing it too.
--
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#discussion_r493362675___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request.
> else:
-dep_normalized_name = dep.key
+dep_normalized_name = dep.key.replace('_', '-')
Why do you still do the `replace` here, but not in the other places where you
use `dep.key`? I'm not saying it's necessarily wrong, but I don't understand it.
--
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-494464867___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava commented on this pull request. > +if isdir(path): +path = dirname(path) +res = re.search(r"/python(?P\d+\.\d+)/", path) This breaks detection of Python version from the name of the egg-info file. And I think you misunderstood me, I wasn't doubting the usefulness of the variable, I was asking why do you do `dirname` at all. ```suggestion res = re.search(r"/python(?P\d+\.\d+)/", path) ``` -- 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-494457340___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
This pull request **fixes 1 alert** when merging 8a5494c5d22b620a071b73d8b3dc31f495a7cdcd into 4cbdd7c94071f52dfe64466df80e859193d89d89 - [view on LGTM.com](https://lgtm.com/projects/g/rpm-software-management/rpm/rev/pr-8556ea7d2c7464263c5c35dacf6ac13c31779659) **fixed alerts:** * 1 for Module is imported more than once -- 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#issuecomment-697163942___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@s-t-e-v-e-n-k commented on this pull request.
> -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())
Because dep.key is from setuptools' pkg_resources.Requirement and
packaging.Requirement does not include it -- I'm not against creating a
subclass of packing.Requirement here and including a key property that returns
self.name.lower()
--
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#discussion_r493154379___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok commented on this pull request. > -names = list(py_deps.keys()) -names.sort() -for name in names: +for name in sorted(list(py_deps.keys())): ```suggestion for name in sorted(py_deps.keys()): ``` -- 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-493603639___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
> Latest push now has every test successfully run the script, however that one > test still fails because there's a difference in behaviour between the script > in this repo and in python-rpm-generators. > > ``` > -if '>' == operator: > -# distutils does not behave this way, but this is > -# their recommendation > -# > https://mail.python.org/archives/list/distutils-...@python.org/thread/NWEQVTCX5CR2RKW2LT4H77PJTEINSX7P/ > +if operator == '>': > +# distutils will allow a prefix match with '>' > operator = '>=' > -version.increment() > +if operator == '<=': > +# distutils will not allow a prefix match with '<=' > +operator = '<' > ``` > > The differences in that block are the cause of the test failure as far as I > can work out. I believe the behaviour has been changed and you have the new correct version, but I'll double check. -- 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#issuecomment-696766915___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok commented on this pull request. > from warnings import warn +try: +from importlib.metadata import PathDistribution +from pathlib import Path +except ImportError: +from importlib_metadata import PathDistribution +from pathlib2 import Path I'd rather have those two split, to use pathlib + importlib_metadata on e.g. Python 3.7. -- 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-493511637___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@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
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava Hi, do you have a chance over the next few days to check this over again? -- 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#issuecomment-686888640___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
This pull request **fixes 1 alert** when merging 783cf4e257acf0b7586e36077259fbd61956a6d3 into a2e072d45243bcfaa7797514bb40c85e9b8fa74c - [view on LGTM.com](https://lgtm.com/projects/g/rpm-software-management/rpm/rev/pr-d357aba1ed1ac9bd07fcaf5be15b04a2d4d9dfe3) **fixed alerts:** * 1 for Module is imported more than once -- 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#issuecomment-682313107___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
Latest push now has every test successfully run the script, however that one test still fails because there's a difference in behaviour between the script in this repo and in python-rpm-generators. ``` -if '>' == operator: -# distutils does not behave this way, but this is -# their recommendation -# https://mail.python.org/archives/list/distutils-...@python.org/thread/NWEQVTCX5CR2RKW2LT4H77PJTEINSX7P/ +if operator == '>': +# distutils will allow a prefix match with '>' operator = '>=' -version.increment() +if operator == '<=': +# distutils will not allow a prefix match with '<=' +operator = '<' ``` The differences in that block are the cause of the test failure as far as I can work out. -- 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#issuecomment-682312470___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
Once I remove the comments, it doesn't parse because the path doesn't contain pythonX.Y, so it can't determine the python version, so we need to do something there anyway. -- 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#issuecomment-681320463___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
> Okay, this new push solves all but one of the test failures, which has the > script failing to parse the requires.txt for pyreq2rpm.tests since it > contains comments. Ah, I was glad that pkg_resources allowed those comments for easier orientation, but we can remove them if we must. -- 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#issuecomment-68048___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
This pull request **fixes 1 alert** when merging e9014874eb5f8ce2b349648d85e468885c4a5cff into 276f61949f24b647129d11b1ecb9dc7bd7c9691b - [view on LGTM.com](https://lgtm.com/projects/g/rpm-software-management/rpm/rev/pr-e6d51d1da04c85f35cff47832c088185c94d6558) **fixed alerts:** * 1 for Module is imported more than once -- 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#issuecomment-680657619___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
Okay, this new push solves all but one of the test failures, which has the script failing to parse the requires.txt for pyreq2rpm.tests since it contains comments. -- 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#issuecomment-680652546___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
Note that the tests are with https://fedoraproject.org/wiki/Changes/PythonExtras which was not yet backported here. Take [something](https://src.fedoraproject.org/rpms/python-rpm-generators/commits/master) before [this commit](https://src.fedoraproject.org/rpms/python-rpm-generators/c/3b1100ba1f5cbf1b1ebde65adec82d13d6cc62a6?branch=master). -- 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#issuecomment-673973395___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
> Any updates on the test suite investigation? Is there anything I can do to > help? @s-t-e-v-e-n-k Apologies, I got buried under different priorities. I'll try to get this in a few days. If you'd like to see on your own, the test suite for `pythondistdeps.py` is currently only in our Fedora repo here: https://src.fedoraproject.org/rpms/python-rpm-generators/blob/master/f/tests (file `test_scripts_pythondistdeps.py` and folder `data/scripts_pythondistdeps`). Apologies for the location, the test suite was rejected here in the RPM repo for complexity. A new upstream repo for Python RPM scripts is planned (but that hasn't been created yet), so I'll be adding it there when possible. -- 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#issuecomment-672061507___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
Any updates on the test suite investigation? Is there anything I can do to help? -- 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#issuecomment-670355509___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
There are test failures, but mainly do to different capitalization of names. Plus I haven't been able to run the comparison test metadata for some reason. I'll take a deeper look. -- 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#issuecomment-662924340___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@torsava PTAL on the code and possibly run it trough the tests. I ma not happy that we would need to rething the bootstrap sequence of setuptools/pyparsing/six/packaging but OTOH setutpools indeed bundles all of those. -- 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#issuecomment-662885087___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
> and you've not noticed because setuptools vendors it. We have noticed ;) -- 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#issuecomment-662882711___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
@hroncok @torsava take a look please :) -- 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#issuecomment-662881689___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
As a further note about the packaging dependency -- pkg_resources has been using it since I think at least version 32 (I'd have to go back and check), and you've not noticed because setuptools vendors it. -- 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#issuecomment-662881474___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
This pull request **fixes 1 alert** when merging 6cf6eda9e0393ee9838c18e157ca325cf1faba7f into 38c03ddb18e86c84d89af695f72442d8365eb64e - [view on LGTM.com](https://lgtm.com/projects/g/rpm-software-management/rpm/rev/pr-f726f2940b4d3607587a2b589114a9a2ec9aebb2) **fixed alerts:** * 1 for Module is imported more than once -- 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#issuecomment-662874680___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] pythondistdeps: Switch to importlib.metadata (#1317)
Stop using the heavy-weight pkg_resources and switch to a subclass of importlib.metadata. This does add a dependency on packaging as well for requirement and version parsing. I have also removed the comments about the attempted rewrite, since I have addressed many of the concerns. You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/1317 -- Commit Summary -- * pythondistdeps: Switch to importlib.metadata -- File Changes -- M scripts/pythondistdeps.py (192) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/1317.patch https://github.com/rpm-software-management/rpm/pull/1317.diff -- 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 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
