Make PipRequirement a subclass of packaging.Requirement #542
Conversation
ianrochapg
force-pushed
the
pip-requirement-subclass
branch
from
b151bfd
to
fde1d48
Compare
ben-alkov
requested review from
brunoapimentel,
eskultety,
taylormadore,
ben-alkov,
ejegrova and
slimreaper35
|
You forgot to run Also, we can probably delete |
cachi2/core/package_managers/pip.py
Outdated
| RequirementParseError, | ||
| pkg_resources.extern.packaging.requirements.InvalidRequirement, | ||
| ) as exc: | ||
| parsed: Sequence[Requirement] = list(_parse_requirements(to_be_parsed)) |
There was a problem hiding this comment.
Doing this effectively just vendors the old pkg_resources code same way as pkg_resources vendors packaging from which it builds upon. IOW you can drop the whole _parse_requirements parsing into a list and instead do plain packaging.Requirement(to_be_parsed). Note that tests will have to be adjusted since we're checking some corner cases that relate to _parse_requirements behaviour which no longer apply to the usage of packaging.Requirement.
| def _standardize_name(name: str) -> str: | ||
| return re.sub("[^A-Za-z0-9.]+", "-", name) | ||
|
|
||
|
|
||
| def _standardize_version(version: str) -> str: | ||
| try: | ||
| return str(packaging.version.Version(version)) | ||
| except packaging.version.InvalidVersion: | ||
| version = version.replace(" ", ".") | ||
| return re.sub("[^A-Za-z0-9.]+", "-", version) |
There was a problem hiding this comment.
Same thing about vendoring with ^these 2 - part of the issue is investigating the proper replacements for these and by quickly going through packaging's docs canonicalize_name and version seem like candidates worth exploring whether they accomplish what we need.
There was a problem hiding this comment.
@ianrochapg I see you pushed a new revision, have you looked at the canonicalize functions I pointed to in my earlier comment to see if it worked and integration tests passed? If so, that would be obviously a win for us because we wouldn't need the 2 helpers you're introducing.
| @@ -1323,9 +1321,9 @@ def from_line(cls, line: str, options: list[str]) -> Optional["PipRequirement"]: | |||
|
|
|||
| requirement.download_line = to_be_parsed | |||
| requirement.options = options | |||
| requirement.package = req.project_name | |||
| requirement.package = _standardize_name(req.name) | |||
There was a problem hiding this comment.
Not objecting to this line but I'm unclear whether packaging.Requirement already doesn't have the name attribute normalized so we could potentially just do req.name, but I admit I'm unsure on this one particularly.
PipRequirement implemented pkg_resources which is very lean and also deprecated. Removing pkg_resources and subclassing directly from packaging.Requirement. setuptools has been removed because it was only used by pkg_resources, which has also been removed. Resolves containerbuildsystem#486 Signed-off-by: Ian Rocha <iarocha@redhat.com>
ianrochapg
force-pushed
the
pip-requirement-subclass
branch
from
fde1d48
to
fe46b30
Compare
|
/ok-to-test |
|
@ianrochapg if you don't plan on not actually working on subclassing |
PipRequirement implemented pkg_resources which is very lean and also deprecated. Removing pkg_resources and subclassing directly from packaging.Requirement.
setuptools has been removed because it was only used by pkg_resources, which has also been removed.
Resolves #486
Maintainers will complete the following section
Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:
/ok-to-test(as is the standard for Pipelines as Code)