Closed TerminalFi closed 4 years ago
Very nice! What would the default fallback value of the discrete
option if none is specified?
Very nice! What would the default fallback value of the
discrete
option if none is specified?
Currently it falls back to False. As I look for <!-- discrete=True -->
(case / quote insensitive) anything else like <!-- discrete -->
would fall back to false. Is that the expected behavior?
Is that the expected behavior?
I think it's fine since it's an optional feature. It would also be good if the default behavior could be overridden in the User configuration of MarkdownTOC.
Very nice! What would the default fallback value of the
discrete
option if none is specified?
I will work on a setting discrete_fallback
which will address this request
Rather, I believe there is no need for a true or false argument. The presence of discrete will indicate it is desired. I’ll update this PR tonight.
@TheSecEng Thank you for creating the PR!!
However, I don't understand the need for a discrete_fallback. Basically, all headings should be reflected in the TOC, and I think this discrete feature is meant to exclude some headings as exceptions. Therefore, I don't think there is a need for discrete
to have a value.
Wouldn't a grammar like the one below be simpler and more necessary, as @tajmone suggested in the original Issue?
<!-- discrete -->
# Heading
@TheSecEng @tajmone
On another topic, is the word discrete
appropriate?
I found it difficult to understand at first glance.
I checked Asciidoctor's discrete headings, but is that a standard way?
How about a comment like this one?
<!-- excluded -->
# Heading
In addition, I think that the word MarkdownTOC
has the effect that the MarkdownTOC plugin can later interpret these tags as meaningful tags, and that it suppresses the conflict with other plugins.
<!-- Excluded from MarkdownTOC -->
# Heading
<!-- MarkdownTOC:excluded -->
# Heading
Are these too much?
The second topic is simply what I thought, the word discrete
was simply unfamiliar to me, so I'm fine with it as long as it's more semantically and more appropriate for others. Please let me know your opinions.
In addition, I think that the word
MarkdownTOC
has the effect that the MarkdownTOC plugin can later interpret these tags as meaningful tags, and that it suppresses the conflict with other plugins.<!-- Excluded from MarkdownTOC --> # Heading
<!-- MarkdownTOC:excluded --> # Heading
Are these too much?
I believe this is the best option
<!-- MarkdownTOC:excluded --> # Heading ``
@TheSecEng Before we move forward with this issue, could you please review the PR I sent you? https://github.com/TheSecEng/MarkdownTOC/pull/1
@naokazuterada Thanks ! I didn't realize you already handling the indentation level.
These Unittests are working locally, I am not sure why they are failing.
I'll let you update it and test before pushing more.
These Unittests are working locally, I am not sure why they are failing.
@TheSecEng Can you merge upstream master branch? There are some fixes for failure on CI.
oh, still failing...but I think it would be fine...
same issue? https://github.com/SublimeText/UnitTesting/issues/77
@TheSecEng don't mind failure on CI, just keep checking in your local.
oh, still failing...but I think it would be fine...
same issue? SublimeText/UnitTesting#77
@TheSecEng don't mind failure on CI, just keep checking in your local.
Doesn't look like the same issue, I also looked at other appveyor issues and couldn't figure it out. Likely something to do with the \t
characters.
It is ready to merge otherwise.
Locally
@naokazuterada, sorry for the late reply .... I agree with your alternative proposal to discrete
— my original proposal was merely based on Asciidoctor, from which the feature was inspired, but anything that would align it better with the MarkdownTOC package (or ST packages in general) should have priority.
@tajmone thanks for reply! I will make a final decision based on your feedbacks.
@TheSecEng thanks for modifying your PR! I will see it to final check and write down docs soon.
Hi @TheSecEng
The added tests seem to fail on Windows (appveyor). It looks as if the tests need to be adjusted to work on Windows and not only on Windows.
Traceback (most recent call last):
File "C:\st\Data\Packages\MarkdownTOC\tests\exclude_heading.py", line 63, in test_exclude_heading_level
self.assert_In(' - level 3', toc)
File "C:\st\Data\Packages\MarkdownTOC\tests\base.py", line 96, in assert_In
self.assertIn(txt, toc_txt)
AssertionError: ' - level 3' not found in '\n\n- level 1\n\t- level 3\n\t\t- level 6'
----------------------------------------------------------------------
REF: https://ci.appveyor.com/project/naokazuterada/markdowntoc/builds/33090269
Perhaps assertRegex
should be used instead of assert_In
and assertNotRegexp
instead of assert_NotIn
.
REF: https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRegex
@naokazuterada let me know if you want a hand with the documentation or a review
I will test that tonight. I guess maybe it’s the line endings being different on those platforms.
@TheSecEng I am a Python n00b so I am not so familiar with the testing framework, but it looks as if the whitespace characters are interpreted literally according to file encoding.
@TheSecEng:
maybe it’s the line endings being different on those platforms.
I don't know Python, but I've experienced some EOL issues in various repositories CI builds due to the LF
vs CRLF
difference between Windows and Linux — by default Git treats source files as native, using the latter EOL on Windows.
In Bash scripts that rely on GREP RegEx, I've solved the issue by using [[:cntrl:]]*$
to match the EOL character, which works for both LF
and CRLF
.
You can find a practical example in this Travis CI Bash script that I wrote to check for trailing whitespace:
I hope this might help.
@naokazuterada unit tests working as expected. Feel free to merge
Thanks for the great work @TheSecEng !
fixes #140
Implements the feature requested in 140 by @tajmone
Discrete Heading are configurable inside of the MD file like such:
<!-- discrete=true -->
or<!-- discrete="true" -->
"
and'
or no quotesPassed the UnitTesting.
Note Unrelated Failing unit test
test_uniquify_id_2