Shift requirement parsing inside Requirement

#179 Merged at 33d5535
Repository
stevenk_
Branch
default
Repository
pypa
Branch
default
Author
  1. Steve Kowalik
Reviewers
Description

This removes split out requirement parts from the init method of Requirement, instead parsing the requirement string itself, and having it call packaging.requirements.Requirement directly. This allows to inject the marker and url (if any) of the requirement into pkg_resources.Requirement. Further, this adds back use of RequirementParseError exception which I noticed had been accidentally dropped earlier.

I understand that this change might be controversial, and I'd be delighted to discuss any concerns you have.

Comments (7)

  1. Jason R. Coombs

    Overall, this seems like a good idea. It begins to harmonize the implementations of pkg_resources.Requirement and packaging.requirements.Requirement.

    What's not indicated in the commits or the description is what problem does this solve? You do say "allow to inject the marker and url of the requirement", which is a good start. What I'd really like to see to accept this is:

    • A description indicating what problem this solves, what new features it adds, what compatibility issues (if any) it introduces and how to cope with them, and how the new functionality is to be used. (edit: you can write up this description in the scm changelog, in CHANGES.rst, or here in the PR and I'll integrate it as part of the release).
    • New tests capturing the new expectations and usage introduced by this change. Although the current test suite doesn't have 100% coverage, new code should.
    • Possibly, updates to the documentation (if relevant) or additions describing how Requirements are to be used.

    Additionally, I'm tempted to say this change doesn't go far enough. If packaging.requirements.Requirement is to become canonical for describing requirements, perhaps pkg_resources should subclass Requirement adding the necessary functionality for its purposes, or even better, that functionality could be implemented by packaging directly such that pkg_resources doesn't need its own implementation at all.

  2. RobertR

    The overarching goal here is to be able to use markers directly in requirements rather than having to do the indirection via extras. This will permit them to be used in setup requirements and test requirements as well.

  3. Jason R. Coombs

    The overarching goal here is to be able to use markers directly in requirements rather than having to do the indirection via extras.

    In that case, accepting this pull request will be harder, as this pull request goes only a third of the way, leaving the following incomplete:

    1. Honoring of markers in install_requires. Users would be able to specify markers in install_requires, tests_require, and setup_requires that would be ignored by easy_install and possibly other installers.
    2. A story and plan for replacing markers in extras_require, which already apply to install_requires and extras.

    Previously my instinct, when it comes to extras_require, was to create known "extras" (e.g. "tests", "setup") to supersede setup_requires and tests_require, and thereby adding marker support. At that point, the 'extras_require' are capable of describing any class of requirements and could possibly even replace install_requires, unifying the description of projects Python requirements into a single field. I suspect this was the vision of extras_require when it was created.

    This approach, however, lies in conflict with the designs of pip and the syntax adopted there for describing markers, which has some deficiencies w.r.t. markers. As each marker can apply to only one requirement, there's no grouping of requirements by purpose or platform, as extras_require allows. The grouping is implicit (i.e. requirements with identical markers are implicitly grouped) and out-of-band (with requirements listed in separate fields or separate requirements files).

    Ultimately, the PyPA seeks to develop a solution that allows for requirements to be declared in an elegant form that reflects the inherent grouping and target for each requirement. To that end, I feel that adding environment marker support to aspects other than extras_require would be a distracting and confusing addition, especially in the absence of a plan for supporting the new metadata formats.

  4. Steve Kowalik author

    What Rob has stated is the overarching goal of the work I'm doing entirely. The goal of this PR itself is to add marker support into pkg_resources.Requirement only, since currently if you make use of pkg_resources.Requirement("name>=2; python_version>='3.2'"), the marker information is lost, since the existing implementation of pkg_resources.Requirement did not support specifying the marker in the init method, and I'd like to work in small pieces so that I don't find myself changing the world and then I'm stuck in a hole when the entire test suite fails. :-)

    Furthermore, this PR solves an existing issue with pkg_resources.Requirement which is invalid requirements will raise packaging.requirements.InvalidRequirement, whereas every downstream user I've seen (pip, and a requirements updater in OpenStack) expect parse_requirements or pkg_resources.Requirement.parse to raise pkg_resources.RequirementParseError, since I clearly forgot how exceptions propagate when I wrote the code in the original PR series.

    I will writing some tests today for this PR that check that markers are parsed correctly and that comparisons between pkg_resources.Requirement instances work correctly.

    I have an in-progress patchset locally that adds marker support to {install,setup}_requires and tests_require along with testing the support in extras_require, that I will be pushing up when I'm happy with it.

  5. Jason R. Coombs

    That does sound good and reasonable. Checkpoints with stability are much appreciated.

    I guess the more important question is, are we deciding at this moment in time that markers are to be bound to requirements, while requirements are grouped into pools (tests, setup, install, other)? Accepting this change begins to cement the idea that markers are bound to requirements rather than to groups of requirements, suggesting that the model presented by extras_require is deprecated and to be superseded by the more explicit and redundant model.

    I'm okay if the answer is yes. I just want to be sure we're cognizant of the implications that creates for reconciling those differences with the existing implementation. It sounds like you're aware of the implications, so I won't obstruct this effort. I do appreciate the continued effort.

  6. RobertR

    So PEP 345 added markers and they were indeed intended to be specific to individual requirements - there's some examples on https://www.python.org/dev/peps/pep-0345/#environment-markers

    setuptools marker implementation using the extras shim was - I presume - some accomodation for complexity of implementation or some such. Whatever the case, its been an ongoing difference between setuptools and the PEPs, and between pip - which does markers per requirement - and setuptools.

    So the answer is IMO - yes, we want to bind markers to requirements. As far as deprecation goes - eventually in the fullness of time, yes, I'd encourage deprecating it, but I think its pretty easy to maintain indefinitely, so we shouldn't rush to that.