Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace deprecated pkg_resources with packaging #105

Merged
merged 4 commits into from
Aug 12, 2024
Merged

Replace deprecated pkg_resources with packaging #105

merged 4 commits into from
Aug 12, 2024

Conversation

oh2fih
Copy link
Contributor

@oh2fih oh2fih commented Aug 9, 2024

Replace pkg_resources.Requirement with packaging.requirements.Requirement.

@oh2fih oh2fih requested a review from madpah as a code owner August 9, 2024 11:42
Replace pkg_resources.Requirement
with packaging.requirements.Requirement.

Signed-off-by: Esa Jokinen <[email protected]>
@madpah madpah self-assigned this Aug 12, 2024
@madpah
Copy link
Owner

madpah commented Aug 12, 2024

Thanks for the contribution @oh2fih. As you can see - all tests are failing - looks like you forgot to commit an updated poetry.lock file - can you include this?

@oh2fih
Copy link
Contributor Author

oh2fih commented Aug 12, 2024

@madpah Thanks for accepting the pipeline. I did not yet really focus on the workflows so much as I could not get their output anyway. The poetry.lock has now been updated, and the workflow is waiting for the approval again so that I can see what else is still failing.

Q000 Double quotes found but single quotes preferred

Signed-off-by: Esa Jokinen <[email protected]>
@oh2fih
Copy link
Contributor Author

oh2fih commented Aug 12, 2024

Although the output looks the same, I still got this typing error:

requirements/requirement.py:249: error: Incompatible types in assignment 
(expression has type "List[List[Union[str, Any]]]", variable has type
"List[Tuple[str, str]]")  [assignment]
                req.specs = specs
                            ^
Found 1 error in 1 file (checked 1 source file)

Should I try to fix this or just blindly # type: ignore as you have done 30 times in this file already? 🤔

This is an example of the conversion I use and its output:

>>> from packaging.requirements import Requirement as Req
>>> import re
>>> specs = []
>>> pkg_req = Req('SomeProject >= 1.2, < 2.0')
>>> for specifier in pkg_req.specifier:
...     spec = re.split('([=<>~]+)', str(specifier), maxsplit=1)
...     spec = list(filter(None, spec))
...     specs.append(spec)
... 
>>> print(specs)
[['<', '2.0'], ['>=', '1.2']]

However I test this the type is List[List[str]] and not List[Tuple[str, str]] as mypy claims:

>>> for specifier in pkg_req.specifier:
...    spec = re.split('([=<>~]+)', str(specifier), maxsplit=1)
...    spec = list(filter(None, spec))
...    print(type(spec))
...    specs.append(spec)
... 
<class 'list'>
<class 'list'>
>>> print(type(specs))
<class 'list'>
>>> for i in specs:
...    print(type(i))
... 
<class 'list'>
<class 'list'>

I'll just ignore this...

@oh2fih
Copy link
Contributor Author

oh2fih commented Aug 12, 2024

Getting closer!

  • ✅ It HAD to be List[Tuple[str, strt]] although it was complaining the opposite.
  • ✅ Now all tests from test_requirements.py are passing.
  • ❌ The test_parser.TestParser.test_requirement_files is failing.

@madpah
Copy link
Owner

madpah commented Aug 12, 2024

@oh2fih - can you also see that the DCO check is not passing https://github.com/madpah/requirements-parser/pull/105/checks?check_run_id=28655859108

@oh2fih
Copy link
Contributor Author

oh2fih commented Aug 12, 2024

Yes, the practice of using Signed-off-by seems a bit difficult to adopt.

It seems the old library used was a bit more flexible with the line formatting and I still need to add some pre-processing. However, I am now able to run the workflows on the fork, so maybe I'll get there.

- Hashable type required. Convert list to tuple.
- Ignore incompatible types.
- Remove comments before passing the line to Req().
- Extras should be in lower case.
- Complete the list of comparison operators with '!'.

Signed-off-by: Esa Jokinen <[email protected]>
@oh2fih
Copy link
Contributor Author

oh2fih commented Aug 12, 2024

@madpah It is always a bit challenging to adjust to the developing environment of a new project, but the tox was actually awesome, enabling running the tests locally without manually installing all the requirements. Now I finally got all the tests to pass, and managed to use solutions that weren't too hacky, I think. 😎

@madpah madpah merged commit 19dddfa into madpah:main Aug 12, 2024
19 checks passed
@oh2fih oh2fih deleted the replace-setuptools-with-packaging branch August 12, 2024 15:13
@oh2fih
Copy link
Contributor Author

oh2fih commented Aug 12, 2024

Had I known, I would have cleaned up the commit history a bit, as now the changelog does not contain what was actually improved, the first commit message being most meaningful compared to the minor adjustments in the end.

@madpah
Copy link
Owner

madpah commented Aug 12, 2024

No worries - this PR is here forever.

Thanks for the contribution @oh2fih - 0.11.0 has been released.

mkniewallner added a commit to mkniewallner/requirements-parser that referenced this pull request Oct 24, 2024
Before madpah#105,
`pkg_resources` was implictly present with `setuptools`. So
`types-setuptools` is not needed (and should probably never have been a
dependency, but only a dev dependency anyway, as it was most likely only
needed locally to verify typing).
mkniewallner added a commit to mkniewallner/requirements-parser that referenced this pull request Oct 24, 2024
Before madpah#105,
`pkg_resources` was implictly present with `setuptools`. So
`types-setuptools` is not needed anymore (and should probably never have
been a dependency, but only a dev dependency anyway, as it was most
likely only needed locally to verify typing).
mkniewallner added a commit to mkniewallner/requirements-parser that referenced this pull request Oct 24, 2024
Before madpah#105,
`pkg_resources` was implicitly present with `setuptools`. So
`types-setuptools` is not needed anymore (and should probably never have
been a dependency, but only a dev dependency anyway, as it was most
likely only needed locally to verify typing).
mkniewallner added a commit to mkniewallner/requirements-parser that referenced this pull request Oct 24, 2024
Before madpah#105,
`pkg_resources` was implicitly present with `setuptools`, so it was
useful to have `types-setuptools` to check typing. But now that the
usage is gone, the typing dependency should not be needed. It probably
should have never been a direct dependency anyway, but only a dev
dependency, as AFAIK it was only useful locally to verify typing, not
when installing the library for consumers.
mkniewallner added a commit to mkniewallner/requirements-parser that referenced this pull request Oct 24, 2024
Before madpah#105,
`pkg_resources` was implicitly present with `setuptools`, so it was
useful to have `types-setuptools` to check typing. But now that the
usage is gone, the typing dependency should not be needed. It probably
should have never been a direct dependency anyway, but only a dev
dependency, as AFAIK it was only useful locally to verify typing, not
when installing the library for consumers.
mkniewallner added a commit to mkniewallner/requirements-parser that referenced this pull request Oct 24, 2024
Before madpah#105,
`pkg_resources` was implicitly present with `setuptools`, so it was
useful to have `types-setuptools` to check typing. But now that the
usage is gone, the typing dependency should not be needed. It probably
should have never been a direct dependency anyway, but only a dev
dependency, as AFAIK it was only useful locally to verify typing, not
when installing the library for consumers.

Signed-off-by: Mathieu Kniewallner <[email protected]>
madpah pushed a commit that referenced this pull request Oct 24, 2024
Before #105,
`pkg_resources` was implicitly present with `setuptools`, so it was
useful to have `types-setuptools` to check typing. But now that the
usage is gone, the typing dependency should not be needed. It probably
should have never been a direct dependency anyway, but only a dev
dependency, as AFAIK it was only useful locally to verify typing, not
when installing the library for consumers.

Signed-off-by: Mathieu Kniewallner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Library relies on deprecated pkg_resources
2 participants