拾
--
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
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
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:
@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
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___
@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___
> 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
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
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
@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):
@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
@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
@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
@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:
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
@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
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
@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
@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
@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
@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,
@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):
+
@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):
+
@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
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
@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:
@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
@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
@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
@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
@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
@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
@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
@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
@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
@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
@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
@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
@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:
@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:
@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:
@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
@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
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
@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:
@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
@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
@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`
@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`?
@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
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
@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 =
@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
> 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,
@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
@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
@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:
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
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
-
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:
> 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
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
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:
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
> 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
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:
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
@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
> 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:
@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
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
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
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,
73 matches
Mail list logo
