Repository: pypa/pip
Pull request: Refactor parse_requirements to be independent of InstallRequirement
Fixes #7746

It's a bit of a big PR, sorry, but the commits make the change in stages, so that hopefully makes it a bit easier to review.

Problem statement:
Make parse_requirements independent of InstallRequirement
**What's the problem this feature will solve?**
The code in pip that currently parses requirements files is tightly coupled to the `InstallRequirement` class, meaning that it cannot be used to parse requirements files for any other purpose. By making `parse_requirements` return instances of a generic `ParsedRequirement` class, it will be possible to use the function for purposes other than creaing `InstallRequirement`s


**Describe the solution you'd like**
Create a new `ParsedRequirement` class that captures the data from a requirements file that describes a requirement, and return it from `parse_requirements`.

This will help in developing pip's new resolver which is likely to use a new `project` abstraction rather than directly using `InstallRequirement`

**Alternative Solutions**
Making the new resolver use `InstallRequirement` directly will make the interface messier, and introduce unnecessary coupling between requirement objects and the resolver.

**Additional context**
Part of the work to tidy up pip's code base in preparation for introducing the new resolver.

Hints from issue discussion:

Commit message: Use a new ParsedRequirement class to communicate between handle_line and parse_requirements

Patch to review:
diff --git a/src/pip/_internal/req/req_file.py b/src/pip/_internal/req/req_file.py
index baf8d03ea2a..0263dc15388 100644
--- a/src/pip/_internal/req/req_file.py
+++ b/src/pip/_internal/req/req_file.py
@@ -33,7 +33,7 @@
 if MYPY_CHECK_RUNNING:
     from optparse import Values
     from typing import (
-        Any, Callable, Iterator, List, NoReturn, Optional, Text, Tuple,
+        Any, Callable, Iterator, List, NoReturn, Optional, Text, Tuple, Dict,
     )
 
     from pip._internal.req import InstallRequirement
@@ -84,6 +84,58 @@
 SUPPORTED_OPTIONS_REQ_DEST = [str(o().dest) for o in SUPPORTED_OPTIONS_REQ]
 
 
+class ParsedRequirement(object):
+    def __init__(
+        self,
+        is_editable,  # type: bool
+        comes_from,  # type: str
+        use_pep517,  # type: Optional[bool]
+        isolated,  # type: bool
+        wheel_cache,  # type: Optional[WheelCache]
+        constraint,  # type: bool
+        args=None,  # type: Optional[str]
+        editables=None,  # type: Optional[str]
+        options=None,  # type: Optional[Dict[str, Any]]
+        line_source=None,  # type: Optional[str]
+    ):
+        # type: (...) -> None
+        self.args = args
+        self.editables = editables
+        self.is_editable = is_editable
+        self.comes_from = comes_from
+        self.use_pep517 = use_pep517
+        self.isolated = isolated
+        self.options = options
+        self.wheel_cache = wheel_cache
+        self.constraint = constraint
+        self.line_source = line_source
+
+    def make_requirement(self):
+        # type: (...) -> InstallRequirement
+        if self.is_editable:
+            req = install_req_from_editable(
+                self.editables,
+                comes_from=self.comes_from,
+                use_pep517=self.use_pep517,
+                constraint=self.constraint,
+                isolated=self.isolated,
+                wheel_cache=self.wheel_cache
+            )
+
+        else:
+            req = install_req_from_line(
+                self.args,
+                comes_from=self.comes_from,
+                use_pep517=self.use_pep517,
+                isolated=self.isolated,
+                options=self.options,
+                wheel_cache=self.wheel_cache,
+                constraint=self.constraint,
+                line_source=self.line_source,
+            )
+        return req
+
+
 class ParsedLine(object):
     def __init__(
         self,
@@ -135,11 +187,11 @@ def parse_requirements(
     )
 
     for parsed_line in parser.parse(filename, constraint):
-        req = handle_line(
+        parsed_req = handle_line(
             parsed_line, finder, options, session, wheel_cache, use_pep517
         )
-        if req is not None:
-            yield req
+        if parsed_req is not None:
+            yield parsed_req.make_requirement()
 
 
 def preprocess(content, skip_requirements_regex):
@@ -166,7 +218,7 @@ def handle_line(
     wheel_cache=None,  # type: Optional[WheelCache]
     use_pep517=None,  # type: Optional[bool]
 ):
-    # type: (...) -> Optional[InstallRequirement]
+    # type: (...) -> Optional[ParsedRequirement]
     """Handle a single parsed requirements line; This can result in
     creating/yielding requirements, or updating the finder.
 
@@ -198,8 +250,9 @@ def handle_line(
             if dest in line.opts.__dict__ and line.opts.__dict__[dest]:
                 req_options[dest] = line.opts.__dict__[dest]
         line_source = 'line {} of {}'.format(line.lineno, line.filename)
-        return install_req_from_line(
-            line.args,
+        return ParsedRequirement(
+            args=line.args,
+            is_editable=False,
             comes_from=line_comes_from,
             use_pep517=use_pep517,
             isolated=isolated,
@@ -212,10 +265,13 @@ def handle_line(
     # return an editable requirement
     elif line.opts.editables:
         isolated = options.isolated_mode if options else False
-        return install_req_from_editable(
-            line.opts.editables[0], comes_from=line_comes_from,
+        return ParsedRequirement(
+            editables=line.opts.editables[0],
+            is_editable=True,
+            comes_from=line_comes_from,
             use_pep517=use_pep517,
-            constraint=line.constraint, isolated=isolated,
+            constraint=line.constraint,
+            isolated=isolated,
             wheel_cache=wheel_cache
         )
 
diff --git a/tests/unit/test_req.py b/tests/unit/test_req.py
index 29a23926257..3ce7cedc934 100644
--- a/tests/unit/test_req.py
+++ b/tests/unit/test_req.py
@@ -48,8 +48,9 @@ def get_processed_req_from_line(line, fname='file', lineno=1):
         opts,
         False,
     )
-    req = handle_line(parsed_line)
-    assert req is not None
+    parsed_req = handle_line(parsed_line)
+    assert parsed_req is not None
+    req = parsed_req.make_requirement()
     req.is_direct = True
     return req
 


Code review comments:
diff_hunk: @@ -33,7 +33,7 @@
 if MYPY_CHECK_RUNNING:
     from optparse import Values
     from typing import (
-        Any, Callable, Iterator, List, NoReturn, Optional, Text, Tuple,
+        Any, Callable, Iterator, List, NoReturn, Optional, Text, Tuple, Dict,
path: src/pip/_internal/req/req_file.py
text: @user1:
```suggestion
        Any, Callable, Dict, Iterator, List, NoReturn, Optional, Text, Tuple,
```