Merge pull request #3125 from qwcode/req_line_numbers

refactor to preserve reporting of original line numbers in requirements files

4 files changed, 162 insertions, 46 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index 7c6d85024..925c3891c 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -37,6 +37,9 @@
 * Fix user directory expansion when ``HOME=/``. Workaround for Python bug
   http://bugs.python.org/issue14768, reported in :issue:`2996`.
 
+* Fixed :issue:`3009`, correct reporting of requirements file line numbers
+  (:pull:`3125`)
+
 
 **7.1.2 (2015-08-22)**
 
diff --git a/docs/reference/pip_install.rst b/docs/reference/pip_install.rst
index ff7d56592..489e0b09f 100644
--- a/docs/reference/pip_install.rst
+++ b/docs/reference/pip_install.rst
@@ -99,6 +99,8 @@ treated as a comment.
 A line ending in an unescaped ``\`` is treated as a line continuation
 and the newline following it is effectively ignored.
 
+Comments are stripped *before* line continuations are processed.
+
 Additionally, the following Package Index Options are supported:
 
   *  :ref:`-i, --index-url <--index-url>`
diff --git a/pip/req/req_file.py b/pip/req/req_file.py
index 4b3f683c6..7a1aa53e7 100644
--- a/pip/req/req_file.py
+++ b/pip/req/req_file.py
@@ -65,7 +65,7 @@ def parse_requirements(filename, finder=None, comes_from=None, options=None,
     :param filename:    Path or url of requirements file.
     :param finder:      Instance of pip.index.PackageFinder.
     :param comes_from:  Origin description of requirements.
-    :param options:     Global options.
+    :param options:     cli options.
     :param session:     Instance of pip.download.PipSession.
     :param constraint:  If true, parsing a constraint file rather than
         requirements file.
@@ -81,12 +81,9 @@ def parse_requirements(filename, finder=None, comes_from=None, options=None,
         filename, comes_from=comes_from, session=session
     )
 
-    lines = content.splitlines()
-    lines = ignore_comments(lines)
-    lines = join_lines(lines)
-    lines = skip_regex(lines, options)
+    lines_enum = preprocess(content, options)
 
-    for line_number, line in enumerate(lines, 1):
+    for line_number, line in lines_enum:
         req_iter = process_line(line, filename, line_number, finder,
                                 comes_from, options, session, wheel_cache,
                                 constraint=constraint)
@@ -94,6 +91,19 @@ def parse_requirements(filename, finder=None, comes_from=None, options=None,
             yield req
 
 
+def preprocess(content, options):
+    """Split, filter, and join lines, and return a line iterator
+
+    :param content: the content of the requirements file
+    :param options: cli options
+    """
+    lines_enum = enumerate(content.splitlines(), start=1)
+    lines_enum = ignore_comments(lines_enum)
+    lines_enum = join_lines(lines_enum)
+    lines_enum = skip_regex(lines_enum, options)
+    return lines_enum
+
+
 def process_line(line, filename, line_number, finder=None, comes_from=None,
                  options=None, session=None, wheel_cache=None,
                  constraint=False):
@@ -267,42 +277,53 @@ def build_parser():
     return parser
 
 
-def join_lines(iterator):
+def join_lines(lines_enum):
+    """Joins a line ending in '\' with the previous line.  The joined line takes on
+    the index of the first line.
     """
-    Joins a line ending in '\' with the previous line.
-    """
-    lines = []
-    for line in iterator:
+    primary_line_number = None
+    new_line = []
+    for line_number, line in lines_enum:
         if not line.endswith('\\'):
-            if lines:
-                lines.append(line)
-                yield ''.join(lines)
-                lines = []
+            if new_line:
+                new_line.append(line)
+                yield primary_line_number, ''.join(new_line)
+                new_line = []
             else:
-                yield line
+                yield line_number, line
         else:
-            lines.append(line.strip('\\'))
+            if not new_line:
+                primary_line_number = line_number
+            new_line.append(line.strip('\\'))
+
+    # last line contains \
+    if new_line:
+        yield primary_line_number, ''.join(new_line)
 
     # TODO: handle space after '\'.
-    # TODO: handle '\' on last line.
 
 
-def ignore_comments(iterator):
+def ignore_comments(lines_enum):
     """
-    Strips and filters empty or commented lines.
+    Strips comments and filter empty lines.
     """
-    for line in iterator:
+    for line_number, line in lines_enum:
         line = COMMENT_RE.sub('', line)
         line = line.strip()
         if line:
-            yield line
+            yield line_number, line
 
 
-def skip_regex(lines, options):
+def skip_regex(lines_enum, options):
     """
-    Optionally exclude lines that match '--skip-requirements-regex'
+    Skip lines that match '--skip-requirements-regex' pattern
+
+    Note: the regex pattern is only built once
     """
     skip_regex = options.skip_requirements_regex if options else None
     if skip_regex:
-        lines = filterfalse(re.compile(skip_regex).search, lines)
-    return lines
+        pattern = re.compile(skip_regex)
+        lines_enum = filterfalse(
+            lambda e: pattern.search(e[1]),
+            lines_enum)
+    return lines_enum
diff --git a/tests/unit/test_req_file.py b/tests/unit/test_req_file.py
index 1e3dbbbe5..d714baacd 100644
--- a/tests/unit/test_req_file.py
+++ b/tests/unit/test_req_file.py
@@ -1,6 +1,6 @@
 import os
 import subprocess
-from textwrap import dedent
+import textwrap
 
 from mock import patch, Mock
 import pytest
@@ -12,7 +12,8 @@ from pip.download import PipSession
 from pip.index import PackageFinder
 from pip.req.req_install import InstallRequirement
 from pip.req.req_file import (parse_requirements, process_line, join_lines,
-                              ignore_comments, break_args_options)
+                              ignore_comments, break_args_options, skip_regex,
+                              preprocess)
 
 
 @pytest.fixture
@@ -33,43 +34,132 @@ def options(session):
         format_control=pip.index.FormatControl(set(), set()))
 
 
+class TestPreprocess(object):
+    """tests for `preprocess`"""
+
+    def test_comments_processed_before_joining_case1(self):
+        content = textwrap.dedent("""\
+          req1 \\
+          # comment \\
+          req2
+        """)
+        result = preprocess(content, None)
+        assert list(result) == [(1, 'req1 req2')]
+
+    def test_comments_processed_before_joining_case2(self):
+        content = textwrap.dedent("""\
+          req1\\
+          # comment
+        """)
+        result = preprocess(content, None)
+        assert list(result) == [(1, 'req1')]
+
+    def test_comments_processed_before_joining_case3(self):
+        content = textwrap.dedent("""\
+          req1 \\
+          # comment
+          req2
+        """)
+        result = preprocess(content, None)
+        assert list(result) == [(1, 'req1 req2')]
+
+    def test_skip_regex_after_joining_case1(self, options):
+        content = textwrap.dedent("""\
+          patt\\
+          ern
+          line2
+        """)
+        options.skip_requirements_regex = 'pattern'
+        result = preprocess(content, options)
+        assert list(result) == [(3, 'line2')]
+
+    def test_skip_regex_after_joining_case2(self, options):
+        content = textwrap.dedent("""\
+          pattern \\
+          line2
+          line3
+        """)
+        options.skip_requirements_regex = 'pattern'
+        result = preprocess(content, options)
+        assert list(result) == [(3, 'line3')]
+
+
 class TestIgnoreComments(object):
     """tests for `ignore_comment`"""
 
-    def test_strip_empty_line(self):
-        lines = ['req1', '', 'req2']
+    def test_ignore_line(self):
+        lines = [(1, ''), (2, 'req1'), (3, 'req2')]
+        result = ignore_comments(lines)
+        assert list(result) == [(2, 'req1'), (3, 'req2')]
+
+    def test_ignore_comment(self):
+        lines = [(1, 'req1'), (2, '# comment'), (3, 'req2')]
         result = ignore_comments(lines)
-        assert list(result) == ['req1', 'req2']
+        assert list(result) == [(1, 'req1'), (3, 'req2')]
 
     def test_strip_comment(self):
-        lines = ['req1', '# comment', 'req2']
+        lines = [(1, 'req1'), (2, 'req # comment'), (3, 'req2')]
         result = ignore_comments(lines)
-        assert list(result) == ['req1', 'req2']
+        assert list(result) == [(1, 'req1'), (2, 'req'), (3, 'req2')]
 
 
 class TestJoinLines(object):
     """tests for `join_lines`"""
 
     def test_join_lines(self):
-        lines = dedent('''\
-        line 1
-        line 2:1 \\
-        line 2:2
-        line 3:1 \\
-        line 3:2 \\
-        line 3:3
-        line 4
-        ''').splitlines()
-
+        lines = enumerate([
+            'line 1',
+            'line 2:1 \\',
+            'line 2:2',
+            'line 3:1 \\',
+            'line 3:2 \\',
+            'line 3:3',
+            'line 4'
+        ], start=1)
         expect = [
+            (1, 'line 1'),
+            (2, 'line 2:1 line 2:2'),
+            (4, 'line 3:1 line 3:2 line 3:3'),
+            (7, 'line 4'),
+        ]
+        assert expect == list(join_lines(lines))
+
+    def test_last_line_with_escape(self):
+        lines = enumerate([
             'line 1',
-            'line 2:1 line 2:2',
-            'line 3:1 line 3:2 line 3:3',
-            'line 4',
+            'line 2 \\',
+        ], start=1)
+        expect = [
+            (1, 'line 1'),
+            (2, 'line 2 '),
         ]
         assert expect == list(join_lines(lines))
 
 
+class TestSkipRegex(object):
+    """tests for `skip_reqex``"""
+
+    def test_skip_regex_pattern_match(self):
+        options = stub(skip_requirements_regex='.*Bad.*')
+        line = '--extra-index-url Bad'
+        assert [] == list(skip_regex(enumerate([line]), options))
+
+    def test_skip_regex_pattern_not_match(self):
+        options = stub(skip_requirements_regex='.*Bad.*')
+        line = '--extra-index-url Good'
+        assert [(0, line)] == list(skip_regex(enumerate([line]), options))
+
+    def test_skip_regex_no_options(self):
+        options = None
+        line = '--extra-index-url Good'
+        assert [(0, line)] == list(skip_regex(enumerate([line]), options))
+
+    def test_skip_regex_no_skip_option(self):
+        options = stub(skip_requirements_regex=None)
+        line = '--extra-index-url Good'
+        assert [(0, line)] == list(skip_regex(enumerate([line]), options))
+
+
 class TestProcessLine(object):
     """tests for `process_line`"""