Refactor Block spans, use line numbers not offsets (jamescooke#216)

CHANGELOG.rst

@@ -28,6 +28,15 @@ See also `latest documentation
   ``--aaa-act-block-style=large``. Also fix up associated Makefile recipes and
   update example README file.
 
+* ⛏️ Remove list of nodes from ``Block`` class and instead keep the start and
+  end line numbers of the block. This allows for any structural discoveries
+  while doing AST node traversal (e.g. when parsing Large style Act blocks) to
+  be used to calculate the size of the Act block itself. The alternative would
+  be to store the list of nodes in the Act block, and then re-walk them when
+  working out the block's span, which would be duplication of effort.
+
+* ⛏️ Remove unused ``MultiNodeBlock``.
+
 0.14.1_ - 2023/04/25
 --------------------
 

examples/good/test_with_statement.py

@@ -1,4 +1,5 @@
 import io
+import pathlib
 import warnings
 from typing import Generator, List
 
@@ -12,22 +13,28 @@ def test_pytest_raises() -> None:
         list()[0]
 
 
-def test_deprecation_warning():
+def test_deprecation_warning() -> None:
     with pytest.deprecated_call():
         warnings.warn("deprecate warning", DeprecationWarning)
 
 
-def test_user_warning():
+def test_user_warning() -> None:
     with pytest.warns(UserWarning):
         warnings.warn("my warning", UserWarning)
 
 
 # --- Use of context managers in tests ---
 
 
-def test_simple(hello_world_path) -> None:
+def test_simple(hello_world_path: pathlib.Path) -> None:
     """
-    `with` statement is part of arrange. Blank lines are maintained around Act.
+    Test checks "simple" context manager in both Act block styles:
+
+    * DEFAULT: `with` statement is part of arrange. Blank lines are maintained
+        around Act.
+    * LARGE: When formatted with Black, context manager is squashed against act
+        node. Large act block mode allows the context manager to join the act
+        block and linting passes.
     """
     with open(hello_world_path) as f:
 
@@ -36,20 +43,33 @@ def test_simple(hello_world_path) -> None:
     assert result == 'Hello World!\n'
 
 
-def test_whole(hello_world_path) -> None:
+def test_whole(hello_world_path: pathlib.Path) -> None:
     """
-    `with` statement wraps whole of test
+    `with` statement wraps whole of test. This checks context manager in both
+    Act block styles:
+
+    * DEFAULT: `with` statement is part of Arrange. Result assignment is Act.
+        Blank lines are maintained.
+    * LARGE: When formatted with Black, context manager is squashed against
+        result assignment. Act block grows to consume context manager because
+        it's the first node in the context manager body. However, new large Act
+        block _still_ finishes at the end of the result assignment. There is
+        then a blank line and the assert block, even though that's inside the
+        context manager.
     """
     with open(hello_world_path) as f:
 
         result = f.read()
 
         assert result == 'Hello World!\n'
+        assert f.fileno() > 0
 
 
-def test_extra_arrange(hello_world_path) -> None:
+def test_extra_arrange(hello_world_path: pathlib.Path) -> None:
     """
-    Any extra arrangement goes in the `with` block.
+    Any extra arrangement goes in the `with` block. Works "as-is" for Large act
+    block style because `f.read()` node prevents the `result = ` act node from
+    joining the `with` context manager.
     """
     with open(hello_world_path) as f:
         f.read()
@@ -59,7 +79,7 @@ def test_extra_arrange(hello_world_path) -> None:
     assert result == ''
 
 
-def test_assert_in_block(hello_world_path) -> None:
+def test_assert_in_block(hello_world_path: pathlib.Path) -> None:
     """
     Any assertion that needs the `with` block open, goes after Act and a BL.
     """
@@ -73,7 +93,7 @@ def test_assert_in_block(hello_world_path) -> None:
     assert result == ''
 
 
-def test_pytest_assert_raises_in_block(hello_world_path) -> None:
+def test_pytest_assert_raises_in_block(hello_world_path: pathlib.Path) -> None:
     """
     Checking on a raise in a with block works with Pytest.
     """
@@ -85,7 +105,7 @@ def test_pytest_assert_raises_in_block(hello_world_path) -> None:
         assert f.read() == 'Hello World!\n'
 
 
-def test_pytest_assert_raises_on_with(hello_world_path) -> None:
+def test_pytest_assert_raises_on_with(hello_world_path: pathlib.Path) -> None:
     """
     Checking on the raise from a with statement works with Pytest.
     """
@@ -96,7 +116,7 @@ def test_pytest_assert_raises_on_with(hello_world_path) -> None:
     assert 'invalid mode' in str(excinfo.value)
 
 
-def test_with_in_assert(hello_world_path) -> None:
+def test_with_in_assert(hello_world_path: pathlib.Path) -> None:
     """
     Using with statement in Assert block is valid
     """

src/flake8_aaa/block.py

@@ -1,5 +1,5 @@
 import ast
-from typing import Iterable, List, Tuple, Type, TypeVar
+from typing import List, Tuple, Type, TypeVar
 
 from .conf import ActBlockStyle
 from .exceptions import EmptyBlock
@@ -13,21 +13,25 @@ class Block:
     """
     An Arrange, Act or Assert block of code as parsed from the test function.
 
-    Act blocks are simply a single Act node in default mode. However, in a
-    future version (update TODO200), "large" Act blocks will include the Act
-    node and any context managers that wrap them.
+    A Block is simply a group of lines in the test function. It has start and
+    end line numbers (inclusive), along with line type.
 
     Note:
-        Blocks with no nodes are allowed (at the moment).
+        All Blocks require at least one line. If an empty block is discovered
+        while building, this is passed as the ``EmptyBlock`` exception.
 
-    Args:
-        nodes: Nodes that make up this block.
+    Attributes:
+        first_line_no: First line of Block inclusive.
+        last_line_no: Last line of Block inclusive..
         line_type: Type of line that this blocks writes into the line markers
             instance for the function.
     """
 
-    def __init__(self, nodes: Iterable[ast.AST], lt: LineType) -> None:
-        self.nodes = tuple(nodes)
+    def __init__(self, first_line_no: int, last_line_no: int, lt: LineType) -> None:
+        assert first_line_no > 0, 'First line before start of file'
+        assert first_line_no <= last_line_no, 'Got last line is before first line of Block'
+        self.first_line_no = first_line_no
+        self.last_line_no = last_line_no
         self.line_type = lt
 
     @classmethod
@@ -43,9 +47,10 @@ def build_act(
         Args:
             node: Act node already found by Function.mark_act()
             test_func_node: Node of test function / method.
-            act_block_style: Currently always DEFAULT. TODO200
+            act_block_style: Currently always DEFAULT.
         """
-        return cls([node], LineType.act)
+        first, last = get_span(node, node)
+        return cls(first, last, LineType.act)
 
     @classmethod
     def build_arrange(cls: Type[_Block], nodes: List[ast.stmt], act_block_first_line: int) -> _Block:
@@ -56,27 +61,40 @@ def build_arrange(cls: Type[_Block], nodes: List[ast.stmt], act_block_first_line
         Args:
             nodes: Body of test function / method.
             act_block_first_line
-        """
-        return cls(filter_arrange_nodes(nodes, act_block_first_line), LineType.arrange)
 
-    def get_span(self, first_line_no: int) -> Tuple[int, int]:
+        Raises:
+            EmptyBlock: When no arrange nodes are found, there is no Arrange
+                Block.
         """
-        Args:
-            first_line_no: First line number of Block. Used to calculate
-                relative line numbers.
+        nodes = filter_arrange_nodes(nodes, act_block_first_line)
+        if not nodes:
+            raise EmptyBlock()
 
-        Returns:
-            First and last line covered by this block, counted relative to the
-            start of the Function.
+        first, last = get_span(nodes[0], nodes[-1])
+        return cls(first, last, LineType.arrange)
 
-        Raises:
-            EmptyBlock: when block has no nodes
-        """
-        if not self.nodes:
-            raise EmptyBlock(f'span requested from {self.line_type} block with no nodes')
-        # start and end are (<line number>, <indent>) pairs, so just the line
-        # numbers are picked out.
-        return (
-            get_first_token(self.nodes[0]).start[0] - first_line_no,
-            get_last_token(self.nodes[-1]).end[0] - first_line_no,
-        )
+
+def get_span(first_node: ast.AST, last_node: ast.AST) -> Tuple[int, int]:
+    """
+    Generate span of a Block as line numbers.
+
+    First and last line covered by first and last nodes provided. The intention
+    is that either:
+
+    * For Blocks with a single node, that node is passed as first and last
+      node. Therefore both nodes are the same and their span is calculated.
+
+    * For Blocks with multiple nodes, the first and last of the Block are
+      checked to provide the span of the Block. The caller has to manage which
+      is the first and last node.
+
+    Args:
+        first_node: First node in Block.
+        last_node: Last node in Block.
+    """
+    # start and end are (<line number>, <indent>) pairs, so just the line
+    # numbers are picked out.
+    return (
+        get_first_token(first_node).start[0],
+        get_last_token(last_node).end[0],
+    )

src/flake8_aaa/function.py

@@ -157,26 +157,24 @@ def mark_def(self) -> int:
         Note:
             Does not spot the closing ``):`` of a function when it occurs on
             its own line.
-
-        Note:
-            Can not use ``helpers.build_footprint()`` because function nodes
-            cover the whole function. In this case, just the def lines are
-            wanted with any decorators.
         """
-        first_index = get_first_token(self.node).start[0] - self.first_line_no  # Should usually be 0
+        first_line_no = get_first_token(self.node).start[0]
+        assert first_line_no == self.first_line_no
+
         try:
             end_token = get_last_token(self.node.args.args[-1])
         except IndexError:
             # Fn has no args, so end of function is the fn def itself...
             end_token = get_first_token(self.node)
-        last_index = end_token.end[0] - self.first_line_no
-        self.line_markers.update(first_index, last_index, LineType.func_def)
-        return last_index - first_index + 1
+        last_line_no = end_token.end[0]
+
+        self.line_markers.update(first_line_no, last_line_no, LineType.func_def)
+        return last_line_no - first_line_no + 1
 
     def mark_act(self, act_block_style: ActBlockStyle) -> int:
         """
-        Finds Act node, calculates its span and marks the associated lines in
-        ``line_markers``.
+        Finds Act node, builds it into an Act block and marks the associated
+        lines in ``line_markers``.
 
         Args:
             act_block_style: Currently only DEFAULT. TODO200
@@ -194,13 +192,11 @@ def mark_act(self, act_block_style: ActBlockStyle) -> int:
         """
         # Load act block and kick out when none is found
         self.act_node = self.load_act_node()
-        self.act_block = Block.build_act(self.act_node.node, self.node, act_block_style)
-        # Get relative line numbers of Act block footprint
-        # TODO store first and last line numbers in Block - use them instead of
-        # asking for span.
-        first_index, last_index = self.act_block.get_span(self.first_line_no)
-        self.line_markers.update(first_index, last_index, LineType.act)
-        return last_index - first_index + 1
+        self.act_block = Block.build_act(
+            node=self.act_node.node, test_func_node=self.node, act_block_style=act_block_style
+        )
+        self.line_markers.update(self.act_block.first_line_no, self.act_block.last_line_no, LineType.act)
+        return self.act_block.last_line_no - self.act_block.first_line_no + 1
 
     def mark_arrange(self) -> int:
         """
@@ -216,23 +212,17 @@ def mark_arrange(self) -> int:
             ValidationError: Marking caused a collision.
             ValueError: No Act block has been marked.
         """
-        # TODO get this from self.act_block
-        act_block_first_index = self.line_markers.types.index(LineType.act)
-        act_block_first_line_number = act_block_first_index + self.first_line_no
-        arrange_block = Block.build_arrange(self.node.body, act_block_first_line_number)
-
-        # First and lass offsets of Arrange block - if block is empty, then
-        # work is done.
+        assert self.act_block is not None
         try:
-            first_index, last_index = arrange_block.get_span(self.first_line_no)
+            arrange_block = Block.build_arrange(self.node.body, self.act_block.first_line_no)
         except EmptyBlock:
             return 0
 
         # Prevent overhanging arrangement, for example in context manager. Stop
         # at line before Act block first line offset.
         return self.line_markers.update(
-            first_index,
-            min(last_index, act_block_first_index - 1),
+            arrange_block.first_line_no,
+            min(arrange_block.last_line_no, self.act_block.first_line_no - 1),
             LineType.arrange,
         )
 

src/flake8_aaa/helpers.py

@@ -153,6 +153,7 @@ def get_last_token(node: ast.AST) -> Token:
 def filter_arrange_nodes(nodes: List[ast.stmt], act_block_first_line_number: int) -> List[ast.stmt]:
     """
     Args:
+        nodes: List of body nodes of test function.
         act_block_first_line_number: Real line number of Act block first line.
 
     Returns: