Summary¶
In my last article, I talked about the break I took to work on some refactoring of the properties system for PyMarkdown. In this article, I talk about getting back to PyMarkdown and my efforts to remove items from the issues list.
Introduction¶
While I have referred to the work to date as the initial release, in my mind it was always a beta release. And for me, the focus of a beta release is to continue to test features, to clean up documentation, and to try and resolve any high priority issues. It is not a time to relax, it is a time to make sure that I work on the issues that will have a definite impact on the PyMarkdown project.
The testing of features is going great! Every so often, I find a small issue that is easily fixed within fifteen minutes of finding it. That part is doing fine. Because of my hard work on the documentation prior to the beta release, the main documents are in good condition. But like the testing of features, there are issues where I believe I can add beneficial information to the documents. In addition, some of the later documents need some extra work to bring them up to the same level as the main documents. And as always, there are issues to diagnose and fix.
And that means a lot of this work will span all three of these areas, with a focus on the last two: documentation and fixing issues.
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 consult the commits that occurred on 12 Jun 2021 between this commit and this commit.
Sometimes, Research Does Not Work Out¶
I know, this section is breaking one of my rules about telling a good story: do not give away the ending. But in this case, I think I have a good reason to break that rule. One thing that I have learned over the years is that while success helps us move forward, we learn the best lessons from when things do not work out.
Start At The Beginning¶
At the start of that week’s work, I was looking at this item from the Issues List:
- test_md026_good_unordered_list_into_atx_into_paragraph should not need to test
- what is the effect of changing the ordering of a blank that closes a list so the blank comes last
Basically, with one small exception, the order in which Markdown elements appear in the Markdown token stream are the order in which they appear in the Markdown document. With one exception: end List tokens and Blank Line tokens.
Due to the nature of the algorithms and the consistency checks used for the parser, I cannot remember a time when those two tokens appeared in what I would consider to be the correct order. There are cases where a Blank Line element closes an opened List element. To me it seems logical that the end List token should come first, followed by the Blank Line token that forced the close. From my point of view, in these cases the List element is closed, and then the Blank Line element is processed. So, from where I sit, the problem is that my “correct” ordering is not being reflected properly in the token stream.
The other interesting piece of information? This is only relevant to producing an accurate token stream for linting. To verify that the Markdown is being parsed properly, the HTML generator uses the token stream to create the correct HTML elements. However, because the Blank Line token has no effect on the HTML that is output, the ordering of these tokens has no effect on the resultant HTML. While that is a bit of a saving grace, it also makes it harder to test for.
Learning from trying to solve other hard issues before, I knew I needed to give myself a firm time limit to solve this one. The time limit I picked was 8 hours. That would give me at least two weekday nights and some time on the weekend to work on this issue. That seemed reasonable.
It was time to dig in and get to work!
Add Content In The Middle¶
As I worked through experiments, the easy part was always getting the exact case mentioned above working. But after I got that scenario test working properly, there were always side effects that I needed to mitigate. I looked at the code, make some adjustment for the current experiment that I was working on, only to find out that I did not have the right limitations on the change that I made.
I know that the core code itself is not fragile. That is one of the first thoughts that I had. Looking into that core code, it seems to be stable and allows me to introduce changes without a lot of side effects. It just seemed to me that every time I made simple experimental changes to the core code to elicit the desired behavior, the leading whitespace was an issue.
That was frustrating, but also bolstering. To be honest, while I believe the project is solidly designed, I am sure that I did not get everything right. I would like to think that I got most things right, but I can only empirically say that I engineered the project to produce the correct results per the GFM Specification. I am okay with knowing that I have some more work to do with leading whitespace handling. It is something I now know that I can work on improve in the future. Something that will not blindside me.
In the end, I am sure I am missing things to do with the whitespace and how it is handled. I am just not sure where to go from there. I tried a handful of different approaches to the problem, but it always came down to whitespace and how it was handled. It was a tough thing to come to an understanding with, but most learning worth doing always comes at a cost.
Wrap Things Up In The End¶
After 9 hours, 8 hours plus a bit of extra time, I called it quits… for now. While I was not able to fix the issue, I was able to learn some valuable things.
The main thing is that I learned is that the leading whitespace handling in the parser needs improvement. With each experiment, the success of the experiment quickly devolved into how well I understood the leading whitespace handling. And while I have coded each line of the project, I must concede that I have lost some of the context of that part of the project. Even after reading my articles about how I dealt with those section of the code, I still cannot grasp how I handled that whitespace. So, to move forward, I need to consider whether I want to take some time and rewrite that leading whitespace handling to provide that extra context. Without it, any fixing of issues dealing with leading whitespace are a no-go.
The second thing that I learned is that Markdown to HTML generators probably have an easier time with their implementations than I do. Getting the parser to generate the correct tokens in the right order is easy. It was a bit finicky to do, but relatively easy. Adapting the HTML generator to work with that output was also easy. When I resolved the ordering issue, some complexity in the HTML generator was removed as it was no longer needed. Each experiment failed due to issues with tracking the whitespace properly, not the HTML that was generated.
That is where I left things with this issue: unresolved. I added some documentation to the Developer’s Document, but since the ordering issue is easily worked around, I decided to leave the existing behavior in place. If I want to try and fix this issue again, I will need to make sure to do a better job on the whitespace handling in the parser, as the success of solving this issue will hinge on my ability to understand and adapt that whitespace handling.
But for now, this issue is pretty much permanently benched. Not time lost on something that did not work, but things learned to improve the project and perhaps try again at a later date.
Cleanup¶
As always, I try and make the projects that I work on better with each iteration, and this week’s work was no different. During the testing that I did in the above section, I noticed that I was getting an error at the end of the test runs. For the ~15% of the time that the occurred, it indicated that the coverage file was not valid. It was an easy enough error to fix; I just re-ran the tests and it “went away”. But that got annoying very quickly.
Looking online, others reported similar behavior with the 5.* series of the
coverage tool. Unfortunately, there did not seem to be a fix in place, only
people reporting that it was intermittent. Based on some of those reports,
the quickest solution seemed to be to go back to the pytest-cov
package’s 2.10.1 version and the coverage package’s 4.5.4 version.
After about 20 test runs of the scenario tests, and many times since then,
that error has not showed up. The tests do seem to run a bit slower when
I am looking for coverage data, but at least that error does not show up
anymore. When I have some time, I will try and look at it some more, but it
is fixed enough to not be a problem.
Other than that, the only change that bears mentioning is that I cleaned up
the file unordered_list_into_atx_into_paragraph.md to make it simpler to
parse instead of containing a long list and a long paragraph. I found out
that I do not need a list with five items and a paragraph with five lines,
I just need a list with one item and one line. I cannot recall if that
added the violation of rule md022 or not, but I added it to the test
that references that file to make sure it was reporting cleanly.
Once again, nothing serious, just some simple cleanup.
Issue 8: Exposing Command Line Options As Configuration Items¶
Issue 8 was created
to address a simple issue: the --stack-trace and --add-plugin command
line arguments did not have a corresponding equivalent in the configuration
system. While it may seem like a trivial change, for me it was an
important one. It is particularly important to me that each command line item should
have a configuration item, unless there was a very good reason, and only
on an item-by-item basis.
These two command line flags were the only reasonable cases that had not been addressed. It was time to fix that.
Code Changes¶
The change for the --stack-trace flag was a simple change at the
start of the __initialize_logging function. Instead of:
self.__show_stack_trace = args.show_stack_trace
this code was added:
self.__show_stack_trace = args.show_stack_trace
if not self.__show_stack_trace:
self.__show_stack_trace = self.__properties.get_boolean_property(
"log.stack-trace"
)
It is a simple change, but a necessary one to follow the configuration ordering detailed in the Advanced Configuration document. Prior to this change, only the command line flag was being checked. With this change, if the command line flag was not used, then the configuration system is queried to determine if the stack trace should be enabled. Because the command line only ever enables the flag, the logic remains simple.
While a slight bit more complex, similar changes were required for
the initialize function in the plugin manager. Originally, the code was
quite simple:
if additional_paths:
for next_additional_plugin in additional_paths:
Basically, if the command line provided any addition plugin paths to
explore, they were placed in the additional_paths variable that
contained an array of paths. The for statement at the end of
the example leads into the code that handles loading plugins when
either a directory or a file is specified. Very simple, and very
straightforward.
This is where the code got more complex, but not unbearably so:
all_additional_paths = []
if additional_paths:
all_additional_paths.extend(additional_paths)
more_paths = properties.get_string_property("plugins.additional_paths")
if more_paths:
all_additional_paths.extend(more_paths.split(","))
if all_additional_paths:
for next_additional_plugin in all_additional_paths:
In the new code, the variable all_additional_paths is the new array
that contains all the paths. First, that array is primed with
anything presented on the command line, as before. Then the configuration
system is checked to see if any additional paths are specified. If
any string is specified, it is treated as a comma-separated list and
processed into elements as such. That list of paths is then added
to the end of the all_additional_paths list, and then processing
resumes as before.
Documentation Changes¶
The code changes to get this working took approximately 40 minutes to
complete, from writing the first scenario test to ensuring that each
test was passing properly. The interesting part was in documenting
the behaviors of these flags. Specifically, the issue was in
documenting the --stack-trace flag versus the log.stack-trace
configuration item.
In both cases, the actual documentation for command line option versus
configuration item took minimal effort. However, in the case of the
--stack-trace flag, there is a significant difference that needed
to be documented.
Prior to this change, the documentation for the flag was limited to this entry in the Advanced Configuration file:
if an error occurs, print out the stack trace for debug purposes. Also sets the initial logging (config processing) to debug. (Default:
false)
While that was true while just the command line flag was present, it also did not go into much depth on what the other effects are. As I wanted to make sure I was being clear, I took my time to properly document the differences between the command line flag and the configuration item.
It was a simple difference, but it took some explaining. Once the logging system was initialized, the behavior of both items was the same. The difference was in the behavior before the logging system was initialized. I needed to ensure that if there was an error logged before the logging system was initialized, that I can diagnose it. Working through those debug scenarios and documenting them in the Advanced Configuration document made sense. A good side effect is that it also validated my design and implementation of the starting code worked.
And Along The Way…¶
Everyone has their pet peeves, and I am not immune from their power either. In my case, I prefer to avoid having to memorize long command lines and various “obscure” sequences in favor of useful helper scripts and looking up the less familiar sequences when I need them. I am not sure whether it is classified as being lazy or being efficient, but I just do not find any value in memorizing things that can be easily addressed but simpler processes.
It is for that reason that I spent the better part of an hour fine tuning
a script I used called ptest.cmd. Before I switched to that script,
I frequently typed pipenv run pytest into my command shell to execute
the scenario tests. If I needed to specify a specific set of tests, I would
use the form pipenv run pytest -k test_prefix to execute any tests starting
with that test_prefix string.
But I was hitting a bit of an issue with that usage pattern. For a long
time, I have executed the tests with the extra configuration provided by
the setup.cfg file:
addopts=--timeout=10
--html=report/report.html
--cov
--cov-branch
--cov-fail-under=90
--strict-markers
-ra
--cov-report xml:report/coverage.xml
--cov-report html:report/coverage
--junitxml=report/tests.xml
But as I noted in the above section on Cleanup, one of the changes I made to address the PyTest coverage issues resulted in a slower execution time. To address this, I wanted to only execute the tests with coverage data if requested, hopefully speeding things up. And after some simple tests, the execution duration without coverage was amazing. It seemed like the duration for an unmeasured set of tests was approximately 25% of the duration for a measured set of tests.
The problem? It meant moving away from general command line execution
and into a script. While I found the command pipenv run pytest to be
simple enough to use all the time, speeding up the execution of those
tests would require me to include five arguments when I wanted to
execute the tests with coverage. That went over my own internal line
of what was acceptable.
Fixing That Issue¶
The logical place to house those changes was in the ptest.cmd script.
To keep my “clean build” script, clean.cmd, clean, I had set up that
script to call into the ptest.cmd script when it came to executing
the tests. So, I had to figure out how to make this script work as
part of the clean build process and part of my normal development
process.
First, I added a new -c flag to trigger whether coverage should be
enabled for the next test run. As that was the main driver for this
change, I wanted to get it out of the way. I also made changes to
GitHub Workflow main.yml file to mirror what I was doing with the
ptest.cmd script.
Starting to use the script in my normal development, I noticed that
the “normal” mode for the script was to not
show anything about the test run unless there were errors. While that
was fine for the clean build scenario, it was less than optimal for
the development scenario. I addressed that by adding a -q flag
to allow the clean.cmd script to specify that previous behavior. If
not specified, the new default behavior was to show all output from
the tests, providing the same output as if the entire pytest command
was entered on the command line.
Finally, I added the -k flag to allow for the passing in of a keyword
to look for with the tests. I knew I was going to have to support this
flag, as it was part of the two base scenarios that I needed to support,
but I wanted to make sure that everything else was cleaned up first.
With everything else in place, the handling of this flag was easily
added.
And while I am sure I will make more fine-tuning to scripts in the future, this was good enough for now!
Issue 9: Better Plugins Support¶
One thing that had been bothering me for a while were the few items from the Issues List that brought together to make up Issue 9. I had thought out the rule plugins far in advance of when I needed them, but I had not applied that same rigor to the exposing of those same plugins to the command line. I felt that I had done okay in exposing the plugins to the command line, but I knew I could do better.
Plugin Lists¶
The first thing on that list was the listing of the available plugins via
the plugins list command. After using that command a few times to check on the
plugins, I was convinced that two enabled columns, enabled (default)
and enabled (current), did not have to take up the space that they did.
After performing some experiments, I found that by specifying each column
title with a newline character in the middle of the title would split the
column content over multiple lines. Therefore, changing the column
titles to enabled\n(default) and enabled\n(current) provided the exact
effect that I was looking for.
Now, when I used that command, I saw this:
ID NAMES ENABLED ENABLED VERSION
(DEFAULT) (CURRENT)
md001 heading-increment, header-increment True True 0.5.0
md002 first-heading-h1, first-header-h1 False False 0.5.0
md003 heading-style, header-style True True 0.5.0
md018 no-missing-space-atx True True 0.5.0
md019 no-multiple-space-atx True True 0.5.0
While I was doing my experiments, I also found that I was not handling the
case of zero matching plugins properly. To be honest, it
just was not being handled at all. To take care of that, I simply added
a check against if show_rows in the __handle_argparse_subparser_list
function, printing out a nicely formatted message if that case was encountered.
Plugins Info¶
The second thing on my list was to elevate the plugins info response to
be at the same level as for the plugins list command. The effort that
I had put in before was easily a place holder, one that now needed to be
replaced. I had two goals for this change: make it look better and display
relevant information to the command line user.
The first part of that was easy. Leveraging my work in the previous section to display thing in columns, I decided that the information would be best presented in a simple two column format. From my point of view, that would nicely take care of cleaning up the display, producing output like:
ITEM DESCRIPTION
Id md001
Name(s) heading-increment,header-increment
Short Description Heading levels should only increment by one level at a time.
Sitting back for a bit and looking at that design, I went through the mental exercise of trying to figure out why a user would want to look at that information. The first candidate was a user that wanted more information on a rule that was being shown in a rule violation line. The second candidate was a user that was looking at a configuration item that they did not recognize in a configuration file.
The information for both cases had some overlap. For the configuration file case, I figured that a user would want to know if the configuration item they see in the configuration file matches a valid configuration item for the rule plugin. For the output violation case, I figured that a user would want to see information about why that violation was raised. I just had to find the right information that would satisfy the user’s needs in both cases.
Without overloading the user, I decided that there was no really good way of displaying the rule information that was included in the rule’s documentation URL. However, displaying that URL so that they could look for more information there made sense to me. In addition, the configuration item case could be somewhat handled in a similar way. If I presented a line detailing what the configuration items were, any further information on those items could also be obtained by referencing the URL.
Combining both designs together, I came up with the following sample of what I wanted the sample output for Rule md001 to look like:
ITEM DESCRIPTION
Id md001
Name(s) heading-increment,header-increment
Short Description Heading levels should only increment by one level at a time.
Description Url https://github.com/jackdewinter/pymarkdown/blob/main/docs/rules/rule_md001.md
Configuration Items front_matter_title
After modifying a scenario test for md001 and adding scenario tests for md023 and the debug rule md999, it was time to write the code.
Making It Work¶
Once I had the sample output defined in the last section, the rest was
easy. I went back to the plugin_manager.py module and the modules for
each plugin rule and added two optional properties: plugin_url and
plugin_configuration. If the plugin rules provided these optional
properties, they would be displayed in the plugins info output, otherwise
they would be silently omitted.
With those two new properties added to the various plugin classes,
I then started to work on the __handle_argparse_subparser_info
function to make it output the information in columns. As the
columnar package took care of the formatting, I just needed to organize
the data into a series of rows, each row containing a title column
and a description column. That was quickly accomplished within
fifteen minutes of work.
Other than some slight issues with adjusting column widths in the test output, everything worked right away. The column adjustments that needed to be made were completed within five minutes, and everything was buttoned down after ten minutes.
While I am not sure it is perfect yet, the output is now definitely something that I can be proud of!
Issue 10: Moving Token Code Into the Token Module¶
Getting ready to wrap things up on Saturday, I decided to look on the list and find an easy item to resolve. Lucky for me I found the issue detailed in Issue 10. While not a large task, it was a task in the direction that I wanted to move more towards in the future.
During the development prior to the beta release, I spend a lot of time getting things working properly. That meant passing the GFM specification example tests as well as my own tests, the ones verified against BabelMark. While I tried to make the best decisions each time, I admit freely that in some cases I added code in a place that was less than optimal, knowing I would probably revisit it later.
The development of rule md022 was one of those cases. To properly track the
number of blank lines before and after a Heading element, the rule needed to
be coded to understand if an end token was a container token, a leaf token,
or (by default) an inline token. To that extent, I added the
__is_container_end_token function and the __is_leaf_end_token function to
the rule_md_022.py module to accomplish that goal. Knowing that it may
not be the best place for those functions, I added an item to the Issues
List for later examination.
Was that the right thing to do? Yes… and no. I added those functions
to the rule_md_022.py module to get things working, as I was focusing on
verifying those rules. I cannot remember exactly what was going through
my head at the time, but knowing that I needed to take a better look at
it when I had the bandwidth, I made a note in the Issues List in
the same commit
as that change. I simply noted that I wanted to double check it later
and went back to focusing on the code that I was working on.
If you ask me for my opinion on whether it was the right thing to do, without more questions I could not properly answer either yes or no. It depends. However, one thing I am certain about. Not fixing it right away and not noting it down for later would be the wrong thing to do. And to be honest, I am okay with knowing that I avoided doing the wrong thing, even if I am not sure if I did the right thing.
What Was My Experience So Far?¶
If I am being honest, I expected to be dragging more about doing project work at this stage of the project than I am. I know that the last 10%-15% of a project is never the most fun part of the project, but it is a critical stage for any project. I know of many projects that have died at the 85% complete stage of the project, for many reasons. From my experience, those “mostly” finished projects result in a project that looks mostly there, but ultimately comes across as unfinished and unpolished. That is not what I want for the PyMarkdown project.
I am not sure where I am in that 15% range, but I know I want to get past the beta-release stage into a normal release cycle. I want to take the time to do things right, document my work along the way, and to ship something that people will use. And convincing people to use something that is mostly done is not as easy as convincing people to use something that is done. Even better, a project that has a track record of fixing issues is one of the hallmarks I use when looking at projects I want to use. And that is where I want to be.
Is that the right mindset to have at this stage of the project? I am not sure. I just know that is where I am at. I want to invest the time to deal with issues and make this a solid application.
That’s just who I am and where I am at.
What is Next?¶
Having started work on improving the Extensions support on Sunday morning, I did not get it finished before I started writing this article on Sunday afternoon. Other than that, not sure yet, so stay tuned!
Comments
So what do you think? Did I miss something? Is any part unclear? Leave your comments below.