Summary¶
In my last article, I continued in my quest to reduce the size of the issues list. In this article, I worked hard to deal with the 33 scenario tests that I skipped in the last set of commits due to time constraints.
Introduction¶
Pure and simple, this week was all about resolving the 33 scenario tests that I marked as disabled from last week. I was mostly confident at the time that the right decision was to mark these tests as disabled and commit the changes I had already made. The question was whether I could fix this issues in five minutes, five hours, or five days. If the answer was anything except five minutes, then disabling the tests was the right thing to do. Otherwise, it would not be the wrong thing to do, I would just feel foolish that it could have been dealt with quickly instead of disabling the tests.
What Is the Audience for This Article?¶
While detailed more eloquently in this article, my goal for this technical article is to focus on the reasoning behind my solutions, rather that the solutions themselves. For a full record of the solutions presented in this article, please go to this project’s GitHub repository and consult the commits between 03 Dec 2020 and 06 Dec 2020.
Cleaning Up from Last Week¶
At the end of the work that I covered in the last article, I marked 33 scenario test functions as disabled. While I felt comfortable in disabling them for a bit, I did not feel comfortable leaving them disabled for too long. It was time to tackle them and get them resolved!
Starting with Html Blocks¶
I needed to start somewhere, and HTML Block elements just seemed like the best place to start. There was no real good reason to choose them other than, in my mind, they were a simple block type. The text in HTML Block is translated to HTML with no changes or interpretation, and the start and the stop sequences for HTML Block are very constrained, but also very simple. It just seemed like the simplest place to start for something that I was concerned about. My concerns? That solving the problems with all 33 failures were going to be a very difficult task to accomplish.
It was during my usual task of research though debugging that I was able to make a quick breakthrough in diagnosing the problem. As a lucky observation, I noticed that when one of the Series M test functions was executed, the test’s Markdown:
1. 1.
<script>
foo
</script>
resulted in an HTML Block element that started between the end of the sublist and
the end of the list. When the end of that list occurred, it caused the HTML Block
element to be closed, leaving the last two lines to be interpreted as a Paragraph
containing the text foo and a Raw HTML element of </script>. By looking at the
Markdown text above, I believed that both lists should have been closed. The
HTML Block start element should have closed both Ordered List blocks as the HTML
Block was not indented enough to keep either List block open. But how was the code
interpreting that Markdown, that was the real question.
Digging into the code a bit, adding extra debug as I went, I was able to soon figure
out that the parser was trying to close the list, but the right type of close was not
occurring.
As I dug into that code, I noticed that I had found a similar problem with the
Thematic Break element and list closures at some point in the past. To solve the
problem with Thematic Breaks, I added Thematic Break specific code in the
__check_for_list_closures function that properly closes the right amount of List
elements if a Thematic Break element occurred within a List element. After going
through this code and the data from the scenario test’s log, I had a theory that
I had another instance where I needed to follow this pattern.
To test that theory, I quickly changed the code in the HtmlHelper class that
checks for the type of HTML block to break out the “is it a HTML start” code from the
“what kind of HTML start is it”. By creating this new is_html_block function,
it allowed me to change the __check_for_list_closures function in the
ListBlockProcessor class to calculate whether the current line was indeed the
start of a HTML Block, storing that value in the is_html_block variable.
With that completed, I changed code that specified is_theme_break as part
of two if conditions to is_theme_break or is_html_block and I was ready to go.
Following my usual pattern of validation scenario tests, I was able to get the new and changed tokens validated quickly, producing the HTML output that I expected. With no other changes, I was able to get all four disabled scenario tests that dealt with HTML Blocks working again.
With those four tests no longer being disabled, the total count of scenario tests to fix was down 4 scenario tests from 33 to 29. It was a good start, but I still had a long way to go. But with momentum on my side, I carried on!
Maintaining the Momentum with Fenced Code Blocks¶
As I had a pattern that worked for one type of block, I decided to try that same pattern
out on the next type of blocks: Fenced Code Blocks. When I did that, I was
pleasantly surprised that following the same pattern led to the same results, with
one exception. In this case, the “is this a Fenced Code Block start” function
already existed as LeafBlockProcessor.is_fenced_code_block, so it was easy to
wire it into the __check_for_list_closures function after assigning it to the
is_fenced_block variable.
The one exception was that there was a small failure in the rendering of the Fenced
Code Block tokens to HTML.
In cases where an empty Fenced Code Block element was present, any extra whitespace
in the Blank Line was being added to the HTML output after the newline character,
instead of just adding the newline character. As this was the only failure, and
(at least to me) it was obvious that this was the issue, I was able to fix it
by adding the exclusion_condition variable and keying off of that variable. The
testing for that change happened almost as quickly as the coding did, and I was
able to resolve that group of four failing scenario tests.
With another 4 tests that were no longer disabled, the disabled test count was now down to 25.
Atx Headings for The Three-peat?¶
Deciding to work on Atx Headings next, I applied the same pattern and got a very good
result. Other than that changes required to isolate the “is this an Atx Heading start”
code into its own function (and its inclusion into the __check_for_list_closures
function), no other Atx Heading code required changes. After validating and modifying
the tokens for the scenarios, all the tests just passed. It was so weird to have
that happen, that I ran the scenario tests again just to be sure, and they were
indeed passing.
To be clear, it was not that I was doubting myself, I just expected a lot more difficulty in resolving these issues. But, with a little bit of work, another 3 tests were removed from the disabled list, bringing the disabled test count down to 22 tests.
Cleaning Up The HTML: Indented Code Blocks¶
After looking at the disabled tests dealing with Indented Code Block elements, it was almost immediately obvious the same pattern would not work with these tests. As far as I could tell from my initial look, the problem was not with the tokens, but in the determination of the looseness of the Lists. In each of the failures, the only difference was whether the items in the List blocks were surrounded by HTML’s Paragraph tags or not. The problem definitely had something to do with looseness.
The GFM Specification’s definition of looseness states:
A list is loose if any of its constituent list items are separated by blank lines, or if any of its constituent list items directly contain two block-level elements with a blank line between them. Otherwise a list is tight. (The difference in HTML output is that paragraphs in a loose list are wrapped in
<p>tags, while paragraphs in a tight list are not.)
And whether I liked it or not, the HTML output was just a little bit off. Not much, but enough. Specifically, it was cases in which the Blank Line token was appearing right after an end List Block token, a case that I felt should not be marked as loose.
Adding a lot of debug and scanning through the log files, I decided to take an approach
of removing the one set of cases that were producing bad results, rather than redesign
the __calculate_list_looseness function from the ground up. As far as I could tell,
everything else was working, just this one case was failing. Therefore, I created the
new __correct_for_me function where I looked for Blank Line tokens, looked for the
end List Block tokens directly before it. At that point, being within the proper list,
the code was then able to make the right determination on the looseness of the list.
Testing that theory took a while to code up, but the testing went by quickly. It
was during that testing when I noticed that the whitespace in the Paragraph Block
tokens was off by one. As the HTML output is checked before the Markdown rehydration
is checked, the issues with the HTML output prevented this issue from being seen.
However, due to the extensive logging I have in place, I was able to quickly deduce
that I had mixed up the passing of the leading_space_length variable to the
__adjust_line_for_list_in_process function with the before_ws_length variable.
After a quick fix and some retesting, everything was now working, and the count of failing tests had fallen by another 2 tests down to 20 failing tests.
The Long Slog: SetExt Headings¶
Working through the previous scenario tests, I was confident looking at the two tests that deal with SetExt Heading elements that I would be able to deal with them quickly. If someone is right with their confidence on something like this, they are told they are right. If they are not right, they are usually told that they had too much hubris. That day, hubris was having a fun time laughing at me.
As it was a Saturday, I was doing things around the house and getting them off our house’s to-do list. In between each of these tasks, I would sit down and work on trying to figure out what the issue was with these tests and how to properly identify them. I was fairly convinced that I needed to be able to detect the start of a SetExt Heading element. I was failing miserably on trying to figure out how to perform that detection. I tried to be smart about the detection, and it failed. I tried to be simple about the detection, and that too failed. And with each attempt, I was just getting more frustrated.
It was mainly as a joke that I decided to add or True to the end of one of the
if statements. At the very most, I thought I might possibly find some information
related to the “detecting the start of a SetExt Heading” issue that I was working on.
I was flabbergasted when the scenario tests I was debugging just started working.
In retrospect, it somewhat makes sense, though I want to dig into that function some
more in the future to verify it. I believe that it worked because SetExt Heading
elements and Paragraph elements are related. To get a SetExt Heading, you start with
a Paragraph element, and you transform it into the SetExt Heading element once the
SetExt Heading sequence is seen after that paragraph on its own line. As such, I
didn’t need to add any extra processing above that of the Paragraph processing to
the first if statement in the __check_for_list_closures function, as the Paragraph
element processing was already sufficient.
In the case of the second if statement, all the other types of Leaf Block element had
already been added, so adding or True just included the one or two remaining types
of leaf blocks that had been missed. As far as I can tell, it was nothing more than
a shortcut.
Regardless of whether my guess is correct or not, the count was now down from 20 disabled scenario tests to 18 scenario tests… and these ones were going to take some time to figure out.
The Last 18¶
I was then down to the last eighteen scenario tests that I needed to get working, and I knew some serious work was going to be involved. I had already been thinking about this for a while, even going as far to contact the CommonMark Discussion List to help understand some of the thorny issues.
Understanding the Scope Of The Problem¶
When all was said and done, these last scenario tests were failing due to my lack of understanding and implementation of the following section of the GFM specification:
Exceptions: 1. When the first list item in a list interrupts a paragraph—that is, when it starts on a line that would otherwise count as paragraph continuation text—then (a) the lines Ls must not begin with a blank line, and (b) if the list item is ordered, the start number must be 1.
It took me a bit to work through it, but here is how I think about it. Take the Markdown:
1. abc
1.
The first line clearly starts a list, but it also opens a paragraph block with the text
abc in it. Because of that open paragraph, the text in line 2 needs to be handled
carefully, as the previously noted exceptions come into play. In this case, because of
section (a) of the exceptions, this Markdown will be interpreted as:
<ol>
<li>abc
1.</li>
</ol>
If there is even 1 non-whitespace character on that second line after the list start identifier, say the Markdown:
1. abc
1. a
then that section of the exceptions is no longer in play, allowing the second line to be a valid list start:
<ol>
<li>abc
<ol>
<li>a</li>
</ol>
</li>
</ol>
However, the harder one for me to understand was section (b) of the exceptions. The impact of that exception is that while the above example works as I would expect, the following Markdown did not:
1. abc
2. a
producing the following HTML
<ol>
<li>abc
2. a</li>
</ol>
But why?
Everything Is Not Perfect¶
I could give many different examples of what is and is not a proper translation, but the example from the discussion forum that sticks in my head a lot is:
1. The project risked missing
its deadline of March
13. A meeting was called.
2. The architect would take flight
457. It was the red eye.
3. She would stay at the Hilton, room
22. It had an ocean view.
First off, this example is plainly an example based in the English language, and I
assumed that equally valid examples could be constructed for any language. Given that
assumption, when I read that Markdown for the first item in the list, it is obvious to
me that the end of the first item in the list refers to March 13., not a sublist
starting at 13 with the contents of A meeting was called.. The context of the rest
of the sentence leaves me with little doubt that the sentence was written that way.
And each of the other two list items left me with the same confidence. Those numbers
were part of the sentences, and thus the paragraphs that the sentences were in, not
the starts of new lists.
But how should the specification handle the codification of this? The concepts of “sentence context” and “looks right” do not apply to the specification. For something to apply to the specification, there needs to be a solid rule that can be followed without fail. In this case, the second exception comes into play:
(b) if the list item is ordered, the start number must be 1.
While it is not perfect, this exception allows the specification to handle the above
cases in a way that it has a solid rule to follow, and hence predictable results. No
guesswork or “sentence context” involved. I believe that @jgm from the discussion
board put it best:
If we were inventing a light markup language from scratch, I’d want to require a blank line before a list or sublist (see Beyond Markdown), for this and many other reasons.
But we’re not, so we need to find a compromise that works.
And that indeed is a viable solution for starting sublists with any number as the
list’s start number. Therefore, if you the previous example with a non-one sublist
start of 2. to be rendered properly, you need to add a newline:
1. abc
2. a
producing the following HTML:
<ol>
<li>
<p>abc</p>
<ol start="2">
<li>a</li>
</ol>
</li>
</ol>
As @jgm said, and as I agree with, it is a solid compromise.
Attacking the Problem¶
The first thing that I did was to validate that this issue applied to both Ordered Lists
and Unordered Lists, which was quickly accomplished. To me, this indicated that I was
going to be making near identical changes to the is_olist_start function and the
is_ulist_start function. As the Order List changes were the most complicated, I
decided to start with those.
The first part of detecting the condition described above was deciding that the current
line being parsed was already marked for a list start. This was already being
performed in the function, so it was an easy change to do some extra processing if the
is_start variable was set. In that case, to narrow down the things that need to be
checked, the first two changes were to set the is_in_paragraph variable to indicate
whether a paragraph block was open, and the at_end_of_line variable to indicate
that there was no more data to process on the line (hence, a blank line).
With those easy changes out of the way, the variable is_first_item_in_list needed to
be set to indicate whether or not the proposed Ordered List start sequence actually
indicated a new List item or a brand-new List block. While lengthy in its description,
the next part of
the algorithm checked, in order, to see if a parent List block contained the Paragraph
element, if it was the same type of List element, if it had the same List character,
and if the start index for the proposed List element was greater than that of a matching
List element already in progress. If any one of those checks failed, the proposed List
start was stored as True in the is_first_item_in_list variable.
From there, the check was relatively easy. After an additional change to set the
is_not_one variable to indicate whether the olist_index_number variable was not
the string 1, the calculation was easy:
if (
is_in_paragraph
and (at_end_of_line or is_not_one)
and is_first_item_in_list
):
is_start = False
Following the exceptions detailed earlier, when a new list start occurs within an
ongoing paragraph (is_first_item_in_list and is_in_paragraph), a further check
is done to to see if the List element would begin with a blank line (at_end_of_line)
or is an Order List start sequence where the start number is not 1 (is_not_one).
How Well Did This Work?¶
The changes documented in the last section were the bulk of the work, and after that the remaining changes were easy to figure out and work on. With the line:
if is_theme_break or is_html_block or is_fenced_block or is_atx_heading or True:
resolving to True in all cases, I removed that line to make things clearer. While
it makes a mess of any displayed differences, it really is only removing that line and
shifting all text text that was under that if statement to the left by four spaces.
After running through some tests, there were some failures with the translation to
HTML. Those failures were all fixed with two lines of code:
elif output_html and output_html[-1] != ParserHelper.newline_character:
output_html += ParserHelper.newline_character
With that completed, all the scenarios were running except scenario test function
test_list_items_282. After working on that for a while, I marked that scenario test
as disabled, to research it and work on it later. Along the way, I also added two
variations of test function test_list_blocks_263 to test specific cases that I thought
would be a problem, both working without any problems.
What Was My Experience So Far?¶
The first thing that came to mind is that I did have an answer to my question from the Introduction section. It took me five days to resolve those disabled tests. More than anything else, that really cemented my feeling that I made the right decision in committing that block of work I had from the previous week, with tests disabled.
While I was still a bit tired after my sinus cold from the previous two weeks, it felt good to get some real solid debugging work done and out of the way. The contrast between this one week’s work and the previous two week’s work was just staggering. It was a real good feeling to get back up to a speed that I know I can easily achieve.
And given that non-cold increase in momentum, along with the recent reduction of items in the issues list, it was nice to see that the finish line is getting visibly closer. While I don’t want to jinx myself by setting a date, and then missing it, I am guessing that I am going to be ready sometime in early 2021, and that is good with me. Believe it or not, I am very much looking forward to it!
What is Next?¶
With some of the hard issues out of the way, I wanted to finish up all the List Block issues and get started on the Block Quotes issues. Here is hoping that I would be able to do that!
Comments
So what do you think? Did I miss something? Is any part unclear? Leave your comments below.