Gruntfuggly / todo-tree

Use ripgrep to find TODO tags and display the results in a tree view
Other
1.42k stars 137 forks source link

Update default regex to not match newlines and improve cleanup Markdown list matching #639

Open baincd opened 2 years ago

baincd commented 2 years ago

I understand that changes to the default regex are rejected. However, this PR does not change the default regex, but rather fixes a couple of issues with the default regex.

Here is the current regex, and the proposed regex as entered in the Settings UI editor image

  1. In the markdown numbered list match, escape the .
    • The unescaped period matched any character, causing lines like 1X TODO to match
  2. Enforce at least 1 space after - or 1.
    • This will fix lines like 1.TODO or -TODO matching, as those are not valid markdown lists
  3. Change the space before ($TAGS) to only match spaces and tabs (not newlines)
    • This fixes #536
baincd commented 1 year ago

Hi @Gruntfuggly , I understand you don't want change the default regex, but I feel this isn't really a change but a fix to the existing default regex. You can see my explanation for my suggested changes above. I have already applied this in my own settings, but I really think this fix could benefit other users and even potentially resolve some outstanding issues.

Either way you decide, thank you for your consideration :smile:

JustOptimize commented 1 year ago

I found an issue with this regex, it is matching tags even if they are not preceded by any comment An example with the ! tag image

I was using this regex for a while and it doesn't have this issue but it matches the tag if it is proceeded by anything even if it is not in the list (//|# ... (//|#|--|;|/\*|^|^[ \t]*(-|\d+.))[^\s]*($TAGS) image

baincd commented 1 year ago

@JustOptimize,

I found an issue with this regex, it is matching tags even if they are not preceded by any comment An example with the ! tag image

I would disagree that this is a bug. I believe this is exactly how it is intended to work - one of the conditions a tag is matched is if the only chars before it on the line is whitespace

To accomplish what I believe you are looking for (to not to match tags preceded by only whitespace), I would suggest trying this modified version of this PR: (//|#|<!--|;|/\\*|^[ \\t]*(-|\\d+\\.)[ \\t])[ \\t]*($TAGS)

Of course you are free to make this modification in your own settings, but this change should not be made to the default regex because as @Gruntfuggly has made perfectly clear the functionality of default regex will not be changed. (This PR only fix bugs in the default regex without changing it's functionality)

GitMensch commented 4 months ago

The changes 1+2 actually are a change that effects current highlighting "intended for better highlighting markdown", while "the main part" 3 does fix a bug. Can you please move 1+2 out to a separate PR in the hope that the main change gets merged to close the linked bug?