feat: split "long code blocks" rule into suggestion and warning#173
Conversation
| - '``` {0,1}({(code-block|code|sourcecode)}){0,1}[^{}\n]*\n(:[^\n]+:[^\n]*\n)*([^:\n]*\n){36,}```' | ||
| - '::: {0,1}({(code-block|code|sourcecode)}){0,1}[^{}\n]*\n(:[^\n]+:[^\n]*\n)*([^:\n]*\n){36,}:::' |
There was a problem hiding this comment.
Two things here (applies to the 25 line one as well).
- In MyST MD it's best practice to separate options from content:
::: {code} text
:foo: bar
Content
Your implementation doesn't quite cover this case - blank lines after the :option: are captured and count for the total number of lines.
- I think you thought of this but it's likely a copy error - but the ``` based rule doesn't ignore ` characters in the repeated section so it can spill over the initial block.
There was a problem hiding this comment.
Thanks @SecondSkoll, these turned out to be non-trivial to fix.
1. I've changed the relevant parts of the rules to:
((:[^\n]+:[^\n]*\n)+\n){0,1}
which requires the options block (if any) to end with a blank line. I tried to also allow no blank line after the options block, but that caused the blank line in the test case to be captured as part of the code - the behaviour we want to avoid.
Maybe there's some way to get this spot on, but it seems fiddly. Since having a blank line is best practice, I think it's good enough to only cover that case. If we really care, we could have a separate rule that flags no-blank-line-after-options. Topic for another day.
2. Ideally, we shouldn't ignore ` in the repeated section. We also shouldn't ignore :, which is the current behaviour. Ignoring : was actually masking that the ``` based rule could spill over: My test case contained this line after a short codeblock:
But 26 lines is NOT acceptable:
Which contains :, and so was sneakily preventing the rule from spilling over 🤯
What we ideally want is for each line in the repeated section to either:
- be blank, or
- not start with
```and not start with:(to prevent the rule from greedily gobbling the options at this point instead of earlier)
Here's what I ended up with:
(\n|(?!```)[^:\n][^\n]*\n){26,}```
And similarly for the : based rule. Edit: I suppose ((?!```)(?!:)[^\n]*\n){26,}``` would work too, and is arguably simpler.
SecondSkoll
left a comment
There was a problem hiding this comment.
Looks great, just pushed the changes we reviewed together.
Resolves #149
Includes a fix from #174 to set
MinAlertLevel = suggestioninvale.ini.