Implementing skip_blank option #37

aroberge · 2022-08-14T20:19:11Z

I've implemented the skip_blank option by adding a new line type, and adding a new Option.
Here's an example where the default (skipping blank lines) is used:

Traceback (most recent call last):
 File "C:\Users\Andre\github\stack_data\example.py", line 13, in <module>
       7 |     length = len(seq)
       9 |     return seq[length]
      12 | seq = [1, 2, 3]
-->   13 | last = last_item(seq)
                  ^^^^^^^^^^^^^^
 File "C:\Users\Andre\github\stack_data\example.py", line 9, in last_item
       5 | def last_item(seq):
       7 |     length = len(seq)
-->    9 |     return seq[length]
                      ^^^^^^^^^^^
IndexError: list index out of range

And here's an example where skip_blank is set to False:

Traceback (most recent call last):
 File "C:\Users\Andre\github\stack_data\example.py", line 14, in <module>
       7 |     length = len(seq)
       : |
      10 |     return seq[length]
       : |
      13 | seq = [1, 2, 3]
-->   14 | last = last_item(seq)
                  ^^^^^^^^^^^^^^
 File "C:\Users\Andre\github\stack_data\example.py", line 10, in last_item
       5 | def last_item(seq):
       6 |
       7 |     length = len(seq)
       : |
-->   10 |     return seq[length]
                      ^^^^^^^^^^^
IndexError: list index out of range

aroberge · 2022-08-14T20:46:59Z

Here's a different possibility when multiple lines are skipped:

Traceback (most recent call last):
 File "C:\Users\Andre\github\stack_data\example.py", line 14, in <module>
       7 |     length = len(seq)
       :
      10 |     return seq[length]
       :
      13 | seq = [1, 2, 3]
-->   14 | last = last_item(seq)
                  ^^^^^^^^^^^^^^
 File "C:\Users\Andre\github\stack_data\example.py", line 10, in last_item
       5 | def last_item(seq):
       6 |
       7 |     length = len(seq)
       :
-->   10 |     return seq[length]
                      ^^^^^^^^^^^
IndexError: list index out of range

I admit that it is a bit of a contrived examples, as there are rarely two blank lines in the code, except before functions and class definitions. Still, I have a slight preference for this one, using only : and not : |, as it is less cluttered and shows better the gap -- I think.

Edit:

Note that I have not made a change to the committed code to reflect this last example.

stack_data/core.py

alexmojaki · 2022-08-14T21:55:27Z

stack_data/core.py

@@ -521,6 +534,10 @@ def __init__(
        self.code = frame.f_code
        self.options = options or Options()  # type: Options
        self.source = self.executing.source  # type: Source
+        if hasattr(self.options, "skip_blank"):


I don't think we need to account for self.options being the wrong type. I also don't think we need a FrameInfo.skip_blank attribute/property anyway - self.options.skip_blank seems fine.

Ok, for not checking that self.options has the correct type. If I use an enum, or something else with three options for skip_blank, I will have to do a test of some kind.

Sorry, I clicked on resolved by accident and forgot about it.

This should now have been taken care of.

stack_data/core.py

stack_data/formatting.py

alexmojaki · 2022-08-14T22:12:49Z

Still, I have a slight preference for this one, using only : and not : |, as it is less cluttered and shows better the gap -- I think.

You could add a Formatter keyword argument that requires a string which will be formatted in place and right-aligned to line up with the | from normal lines, so ": " (that's two spaces at the end) and ": |" would both be allowed and satisfy both your ideas. Some might also want to use a proper unicode vertical ellipsis with 3 dots.

aroberge · 2022-08-14T23:04:30Z

You could add a Formatter keyword argument that requires a string which will be formatted in place and right-aligned to line up with the | from normal lines, so ": " (that's two spaces at the end) and ": |" would both be allowed and satisfy both your ideas. Some might also want to use a proper unicode vertical ellipsis with 3 dots.
Ok. Since this symbol would precede an empty line, there would b no need to add spaces after ":".

alexmojaki · 2022-08-14T23:05:53Z

btw skip_blank might not be a good name if there are three modes.

…efactoring.

…ring needed.

…wing line numbers.

aroberge · 2022-08-16T02:26:27Z

While everything seems to work correctly, more improvements to the code needs to be done. I will try to finish it tomorrow.

…osmetic bug fix.

aroberge · 2022-08-16T10:45:29Z

Other than missing unit tests, I believe that this is now ready. To illustrate the effect of each option, I will use the following example which includes many blank lines:

import stack_data
stack_data.Formatter().set_hook()

def last_item(seq):

    length = len(seq)


    return seq[

        length
    ]


seq = [1, 2, 3]
last = last_item(seq)

Default output

The default is the current behaviour of stack_data, with a small cosmetic fix explained near the end of this comment.

Traceback (most recent call last):
 File "C:\Users\Andre\github\stack_data\example.py", line 16, in <module>
       6 |     length = len(seq)
       9 |     return seq[
      10 |
      11 |         length
      12 |     ]
      15 | seq = [1, 2, 3]
-->   16 | last = last_item(seq)
                  ^^^^^^^^^^^^^^
 File "C:\Users\Andre\github\stack_data\example.py", line 9, in last_item
       4 | def last_item(seq):
       6 |     length = len(seq)
-->    9 |     return seq[
                      ^^^^
      10 |
      11 |         length
               ^^^^^^^^^^
      12 |     ]
               ^
IndexError: list index out of range

Making empty lines "visible" as text.

Setting the following parameters:

options = stack_data.Options(blank_lines=stack_data.BlankLines.VISIBLE)
stack_data.Formatter(options=options).set_hook()

The result is as follows:

Traceback (most recent call last):
 File "C:\Users\Andre\github\stack_data\example.py", line 17, in <module>
       7 |     length = len(seq)
       8 |
       9 |
      10 |     return seq[
      11 |
      12 |         length
      13 |     ]
      14 |
      15 |
      16 | seq = [1, 2, 3]
-->   17 | last = last_item(seq)
                  ^^^^^^^^^^^^^^
 File "C:\Users\Andre\github\stack_data\example.py", line 10, in last_item
       5 | def last_item(seq):
       6 |
       7 |     length = len(seq)
       8 |
       9 |
-->   10 |     return seq[
                      ^^^^
      11 |
      12 |         length
               ^^^^^^^^^^
      13 |     ]
               ^
IndexError: list index out of range

Using only line number "markers" to indicate blank lines.

Setting the following parameters:

options = stack_data.Options(blank_lines=stack_data.BlankLines.LINE_NUMBER)
stack_data.Formatter(options=options).set_hook()

gives the following result (notice that : is used to indicate more than one blank line being skipped).

Traceback (most recent call last):
 File "C:\Users\Andre\github\stack_data\example.py", line 17, in <module>
       7 |     length = len(seq)
       :
      10 |     return seq[
      11 |
      12 |         length
      13 |     ]
       :
      16 | seq = [1, 2, 3]
-->   17 | last = last_item(seq)
                  ^^^^^^^^^^^^^^
 File "C:\Users\Andre\github\stack_data\example.py", line 10, in last_item
       5 | def last_item(seq):
       6 |
       7 |     length = len(seq)
       :
-->   10 |     return seq[
                      ^^^^
      11 |
      12 |         length
               ^^^^^^^^^^
      13 |     ]
               ^
IndexError: list index out of range

Setting a different string for blank lines that are "skipped".

Using the following parameters

options = stack_data.Options(blank_lines=stack_data.BlankLines.LINE_NUMBER)
stack_data.Formatter(options=options, line_number_gap_string=": |").set_hook()

gives the following result:

Traceback (most recent call last):
 File "C:\Users\Andre\github\stack_data\example.py", line 17, in <module>
       7 |     length = len(seq)
       : |
      10 |     return seq[
      11 |
      12 |         length
      13 |     ]
       : |
      16 | seq = [1, 2, 3]
-->   17 | last = last_item(seq)
                  ^^^^^^^^^^^^^^
 File "C:\Users\Andre\github\stack_data\example.py", line 10, in last_item
       5 | def last_item(seq):
       6 |
       7 |     length = len(seq)
       : |
-->   10 |     return seq[
                      ^^^^
      11 |
      12 |         length
               ^^^^^^^^^^
      13 |     ]
               ^
IndexError: list index out of range

Added explanation for a comment.

Using the default values, here's what happen if I were to remove the following code in range_from_node:

        if range_start == range_end == 0:
            # This is an empty line. If it were included, it would result
            # in a value of zero for the common indentation assigned to
            # a block of code.
            return None

Here is the result:

Traceback (most recent call last):
 File "C:\Users\Andre\github\stack_data\example.py", line 16, in <module>
       6 |     length = len(seq)
       9 |     return seq[
      10 |
      11 |         length
      12 |     ]
      15 | seq = [1, 2, 3]
-->   16 | last = last_item(seq)
                  ^^^^^^^^^^^^^^
 File "C:\Users\Andre\github\stack_data\example.py", line 9, in last_item
       4 | def last_item(seq):
       6 |     length = len(seq)
-->    9 |     return seq[
                      ^^^^
      10 |
      11 |         length
           ^^^^^^^^^^^^^^
      12 |     ]
           ^^^^^
IndexError: list index out of range

Notice how, due to the blank line 10 in the marked executing piece, the markers on lines 11 and 12 extend all the way back to the first column.

Minor cosmetic bug fix

In Formatter.format_line, I now have the following:

                # if end <= start, we have an empty line inside a highlighted
                # block of code. In this case, we need to avoid inserting
                # an extra blank line with no markers present.
                if end > start:
                    result += (
                            " " * (start + len(prefix))
                            + self.executing_node_underline * (end - start)
                            + "\n"
                    )

If the end > start test is removed, this is what happens:

Traceback (most recent call last):
 File "C:\Users\Andre\github\stack_data\example.py", line 16, in <module>
       6 |     length = len(seq)
       9 |     return seq[
      10 |
      11 |         length
      12 |     ]
      15 | seq = [1, 2, 3]
-->   16 | last = last_item(seq)
                  ^^^^^^^^^^^^^^
 File "C:\Users\Andre\github\stack_data\example.py", line 9, in last_item
       4 | def last_item(seq):
       6 |     length = len(seq)
-->    9 |     return seq[
                      ^^^^
      10 |

      11 |         length
               ^^^^^^^^^^
      12 |     ]
               ^
IndexError: list index out of range

Notice the gap between lines 10 and 11 in the marked piece.

alexmojaki

Thanks, this is looking pretty good. My points are minor, the biggest remaining thing is the tests.

stack_data/core.py

@@ -521,6 +534,10 @@ def __init__(
        self.code = frame.f_code
        self.options = options or Options()  # type: Options
        self.source = self.executing.source  # type: Source
+        if hasattr(self.options, "skip_blank"):


stack_data/core.py

Co-authored-by: Alex Hall <alex.mojaki@gmail.com>

tests/golden_files/block_left_new.txt

aroberge · 2022-08-20T11:10:28Z

The code coverage has slightly decreased due to:

The addition of a new ValueError exception that could be raised if Options is not initialized correctly.
The absence of testing for the BlankLines.SINGLE when an exception is not raised.

Are unit tests needed for these two cases?

alexmojaki · 2022-08-21T22:50:08Z

The code coverage has slightly decreased due to:

The addition of a new ValueError exception that could be raised if Options is not initialized correctly.

The absence of testing for the BlankLines.SINGLE when an exception is not raised.

Are unit tests needed for these two cases?

The first point seems trivial to test. Just construct a bad formatter and assert the raised exception. No need to do any formatting.

I don't know what the second point means.

alexmojaki · 2022-08-21T22:45:00Z

stack_data/formatting.py

@@ -168,7 +179,7 @@ def format_line(self, line: Line) -> str:
            result += " "

        if self.show_linenos:
-            result += "{:4} | ".format(line.lineno)
+            result += self.line_number_format_string.format(line.lineno)


Why this new option? It seems unrelated to the rest of the PR. And shouldn't it be used in format_blank_lines_linenumbers?

I added it as the indentation was fixed and I might want a larger value in friendly_traceback; it is the only hard-coded value you had in stack_data, while all the others could be changed by the users. And, yes, I overlooked the fact that it should have been used in format_blank_lines_linenumbers as well. I will need to do this.

alexmojaki · 2022-08-21T22:48:37Z

stack_data/formatting.py

+        if self.current_line_indicator:
+            result = " " * len(self.current_line_indicator)
+        else:
+            result = "   "


https://coveralls.io/builds/51814297/source?filename=stack_data%2Fformatting.py#L214 points out that this line is uncovered, and it seems worth testing.

Ok. I probably won't have time to do this tonight though.

I had added this else part based on the presence of the second term in

stack_data/stack_data/formatting.py

Line 173 in 7e88082

result = result or " "

However, it turns out that result was never null and that this line was redundant in the original code, based on all existing tests. So I removed the corresponding else part of my addition.

It's pretty easy to see when the line isn't redundant in the previous code: when you set show_linenos=False, current_line_indicator="". result won't be null, it'll be an empty string. Defaulting it to spaces ensures that the user's code is indented like in a standard traceback. I guess it should have been tested before, but coverage didn't see the gap.

Do you want me to reinstate it and write a relevant test for it?

Yes please.

Looks like there's been some confusion or misremembering. The test needs to specifically have both the arguments show_linenos=False, current_line_indicator="" in one formatter.

Yes, I was wondering why the line result = result or " " did not seem to have any effect. I will fix this.

aroberge · 2022-08-22T00:01:56Z

The absence of testing for the BlankLines.SINGLE when an exception is not raised.

Are unit tests needed for these two cases?

The first point seems trivial to test. Just construct a bad formatter and assert the raised exception. No need to do any formatting.

I don't know what the second point means.
I'm thinking of something like print_stack when many blank lines are printed. I didn't try it out to confirm that it would show changes. I'll need to do some explicit tests to confirm that a unit test might be needed.

alexmojaki · 2022-08-26T21:34:22Z

tests/test_formatter.py

+    try:
+        MyFormatter(show_linenos=False, options=Options(blank_lines=BlankLines.SINGLE))
+    except ValueError:
+        assert True
+    else:
+        assert False


Suggested change

try:

MyFormatter(show_linenos=False, options=Options(blank_lines=BlankLines.SINGLE))

except ValueError:

assert True

else:

assert False

with pytest.raises(ValueError):

MyFormatter(show_linenos=False, options=Options(blank_lines=BlankLines.SINGLE))

and an import pytest at the top.

alexmojaki · 2022-08-26T21:49:19Z

stack_data/formatting.py

@@ -166,12 +177,13 @@ def format_line(self, line: Line) -> str:
            else:
                result = " " * len(self.current_line_indicator)
            result += " "
+        else:


I think this should be removed, the original line result = result or " " is enough. A test without current_line_indicator but with show_linenos would confirm.

You are, of course, correct. I had a brain freeze when I added this.

I've tested this by creating two additional tests (with or without blank lines included). Do you want these extra tests added in a commit?

If you've already written the test etc. then might as well. But no need for extra effort. Just remove these two lines, unless the tests show otherwise.

With the two lines, the result is as follows:

Traceback (most recent call last): File "formatter_example.py", line 85, in blank_lines 79 | def blank_lines(): 80 | a = [1, 2, 3] 82 | length = len(a) 85 | return a[length] ^^^^^^^^^ IndexError: list index out of range

Without them, the indentation is slightly less

Traceback (most recent call last): File "formatter_example.py", line 85, in blank_lines 79 | def blank_lines(): 80 | a = [1, 2, 3] 82 | length = len(a) 85 | return a[length] ^^^^^^^^^ IndexError: list index out of range

Which one do you prefer? Once decided, it probably would make sense to include the tests.

Oh, it looks nicer with the extra spaces, I didn't expect that. Especially if the line numbers reach 4 digits. So let's go with the former option, and then I think result = result or " " isn't needed.

I needed to modified the code when using blank_lines.HIDDEN with no current line marker so that it would be consistent with the others. I added three tests, one for each setting.

…no current line marker.

alexmojaki

All looks good, thanks! Happy for me to merge?

aroberge · 2022-08-27T11:14:47Z

Yes, please do!

aroberge added 5 commits August 14, 2022 08:03

Fixing bug for block highlighting when blank line is present.

00ee1ec

Implementing skip_blank option.

ff09c3a

Merge branch 'alexmojaki:master' into master

9a876de

Removing changes introduced in formatting.py to handle blank lines.

92cbe39

Implementing skip_option by adding new line type.

89621e9

alexmojaki requested changes Aug 14, 2022

View reviewed changes

aroberge added 4 commits August 15, 2022 22:16

Introduced Enum for blank line options. Only names change, no other r…

139fa09

…efactoring.

WIP: added "visible" blank lines. Everything seem to work but refacto…

4a941e3

…ring needed.

WIP: Simplified code by having EmptyLine being a subclass of Line.

4e33238

Ensuring that BlankLines.LINE_NUMBER option can only be used when sho…

c0087c3

…wing line numbers.

aroberge added 2 commits August 16, 2022 00:08

Code refactoring/simplification.

00452f0

Implementing suggestions made in PR comments; minor refactoring and c…

b579c8c

…osmetic bug fix.

aroberge requested a review from alexmojaki August 16, 2022 11:08

alexmojaki requested changes Aug 17, 2022

View reviewed changes

aroberge and others added 5 commits August 17, 2022 09:27

Removing redundant word.

f03e1f3

Co-authored-by: Alex Hall <alex.mojaki@gmail.com>

Removing unnecessary intermediate list.

9449bc5

Co-authored-by: Alex Hall <alex.mojaki@gmail.com>

Addressing comments on previous commit.

274a480

Removing unnecessary EmptyLine class and renaming Enum

7ce94c7

Modifying one existing test to deal with empty lines in executing piece.

7a53114

aroberge commented Aug 19, 2022

View reviewed changes

tests/golden_files/block_left_new.txt Show resolved Hide resolved

Added unit tests.

e42b3c4

Adding line number formatting string as variable

98fde77

alexmojaki requested changes Aug 21, 2022

View reviewed changes

aroberge added 4 commits August 21, 2022 22:15

Replacing hard-coded string by line_number_format_string

f28f550

Removing "case" that was never used in original code.

b17cdc2

Increased test coverage

458d31c

Adding one final test (?)

f15e0c0

alexmojaki reviewed Aug 26, 2022

View reviewed changes

Fixing unit tests.

af46291

alexmojaki reviewed Aug 26, 2022

View reviewed changes

Modified code and dded three tests (for SINGLE, VISIBLE, HIDDEN) for …

4cec1b7

…no current line marker.

alexmojaki approved these changes Aug 27, 2022

View reviewed changes

alexmojaki merged commit 0b69516 into alexmojaki:master Aug 27, 2022

Implementing skip_blank option #37

Implementing skip_blank option #37

Uh oh!

Conversation

aroberge commented Aug 14, 2022

Uh oh!

aroberge commented Aug 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Edit:

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexmojaki commented Aug 14, 2022

Uh oh!

aroberge commented Aug 14, 2022

Uh oh!

alexmojaki commented Aug 14, 2022

Uh oh!

aroberge commented Aug 16, 2022

Uh oh!

aroberge commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Default output

Making empty lines "visible" as text.

Using only line number "markers" to indicate blank lines.

Setting a different string for blank lines that are "skipped".

Added explanation for a comment.

Minor cosmetic bug fix

Uh oh!

alexmojaki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aroberge commented Aug 20, 2022

Uh oh!

alexmojaki commented Aug 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aroberge commented Aug 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aroberge commented Aug 14, 2022 •

edited

Loading

aroberge commented Aug 16, 2022 •

edited

Loading