adding toc_height to exclude upper headings, proposal for #786 and #637

[klml]

I added a new config value toc_height. Using an array for toc_depth seems to me very unlikely.

test local

~/devel/markdown$ python3 -m unittest discover tests
... 550 tests in 1.259s
OK (skipped=71)

[waylan]

Thanks for your contribution.

In addition to the concerns mentioned below, I'm not sure "height" is the right word to describe this feature. To me a "height" of 4 means there are four levels, but that is not what the example int he tests shows. Perhaps that is why I'm struggling to understand the documentation. I don't actually understand what you are trying to do.

Both examples in the tests simply skip level 1. Perhaps one of them should do something other than that. As reminder, this is a feature that I find completely unnecessary, so perhaps I'm missing something obvious here. But if I am, then other users could be as well, which suggests the name and/or docs could be improved.

[klml]

@waylan thanks for your quick response.

You are right about the word height. I was unsure about the naming of toc_height. I wanted to name it the opposite of toc_depth, and I was undecided between toc_height and toc_top.
You mentioned skip, but toc_depth also skips headings (at the bottom). Do you have an idea what word would be meaningful?

Sorry for all these other issues, I will get into it and get back to you with an update on my PR.

[klml]

I fixed the flake error and changed the first test to a higher toc_height.
I tried to improve the wording. But I have a little subjective arbitrability, so do you think it is now more understandable?

I am still struggling with the parameters name toc_height. My alternative shortlist:

• toc_height is missleading, 'a "height" of 4 means there are four levels'.
• toc_skip toc_depth also skips headings (at the bottom).
• toc_top
• toc_edge
• toc_begin
• toc_baselevel is very long and potentialy mixup with baselevel
• toc_startlevel is very long and potentialy mixup with baselevel

@waylan I know you find this feature "completely unnecessary", so I am very glad that you are dealing with me;)
But I think you, as maintainer, should decide which phrase fits best in the whole project.

[waylan]

Your recent changes are a big improvement. They helped bring the new feature into focus for me. Thank you.

I agree that toc_height feels like the wrong name for this. Personally I would favor toc_top.

However, adding this feature introduces another problem, in that toc_depth is now misleading. A user might reasonably assume that the word "depth" refers to the total number of levels. So for example, she might expect the following settings: toc_top=2, toc_depth=2 to result in a TOC which included levels h2 and h3 for a depth of 2. But, in fact, toc_depth limits the bottom to h2 and the "depth" (or number of levels) of the TOC is no longer 2, but 1. Previously, when the "top" couldn't be altered, "depth" and "bottom" were always synonymous. But once the "top" is altered, "depth" and "bottom" are not the same any more. Therefore, toc_depth is a misleading name for the setting as it currently behaves.

I see a few ways forward:

1. Alter toc_depth to be the actual "depth" (number of levels) of the TOC. Perhaps do something like bottom = top + depth - 1 (so for toc_top=2, toc_depth=2, bottom would be 3). For existing users who are not using toc_top, this changes nothing in the actual output. And as toc_top is new, the new behavior of toc_depth will be taken into consideration for any users of the new toc_top setting.
2. Leave toc_depth behavior as-is, but deprecate the name and rename it to toc_bottom. This would be the most disruptive to existing users, which makes it my least favorite option. But having the two settings names "top" and "bottom" is attractive.
3. Implement a range instead. If toc_depth is an integer, maintain the old behavior. If toc_depth is a range ('t-b' where t and b are integers), then treat the range as top and bottom.

Both for options 2 and 3, we would also need to handle the case where a user assigns values such that bottom < top. Of course, that would be an error. But in option 1 that is never a factor. As long as depth is any whole number between 1-6, we will always get bottom > top. Of course, as HTML never drops below h6 we don't actually care if bottom > 6.

[klml]

I like 3. best. With toc_depth optional as integer (as it was) or as range.

But t-b is not actually a data type, but a division. Could I use a tuple (toc_depth = 2, 6) instead in this case?

[waylan]

As a reminder, users don't always provide config settings in Python. Sometimes they might be using a YAML file or similar. So specifically requiring a Python tuple might be a stretch. However, any iterable of length 2 might work.

Personally I was thinking a string with the format "t-b" would make the most sense. If an integer is provided, do old behavior. If a string is provided, it must be two integers separated by a hyphen. Anything else would be an error.

[klml]

I implemented the optional range for toc_depth. I tried to find a more elegant method than two if statements, but I hope this is ok.

[waylan]

Just a few tweaks and this should be good.

[klml]

Using "try" is a good idea, sorry I missed this before.

• I separated and duplicated the continue if clause, so we don't need to define toc_top = 1 in the integer case.
• I moved the split from the loop over all headings to outside.
• I defined the except for a TypeError and a ValueError. Should I use an global except to catch both?

I also thougt about using the AttributeError with the integer case:

try:
    toc_top, toc_bottom = self.toc_depth.split('-')
    if int(el.tag[-1]) < int(toc_top) or int(el.tag[-1]) > int(toc_bottom):
        continue
except AttributeError:
    if int(el.tag[-1]) > int(self.toc_depth):
        continue
except ValueError:
    pass

I am not sure what is better.

And that last line should perhaps be wrapped in its own try/except block to catch and raise an error if a bad range is provided.

At the moment I have no error handling, but a wrong value just gives you the whole Table of Contents and behaves like the toc_depth is not set at all.

I think a warning should be very useful. But as long as I can see, there is no explicit error or warning-handling in Python-Markdown. When I set toc_depth to "foobar" in the existing code I get ValueError: invalid literal for int() with base 10: 'foobar'. Or did I missunderstood you?
When I resolve this, I get also these codecov.io errors.

Thank you for your patience with this issue;)

[waylan]

This looks good. Thank you. It just needs an item added to the 3.1 release notes under "New Features".