aaron-bond / better-comments

https://marketplace.visualstudio.com/items?itemName=aaron-bond.better-comments
MIT License
837 stars 162 forks source link

Javascript: Behavior between inline and multiline comments inconsistent #454

Open aarondill opened 1 year ago

aarondill commented 1 year ago

Issue:

Multiline and inline comments have seemingly opposite behaviors when choosing styles with multiple tags.

Expected outcome:

Coloring of the two situations to be the same if the same tags are used.

Actual outcome:

Multiline comments use the first tag found to decide the styling, while inline comments chose the last tag found before a whitespace.

Environment:

Extension Version(Last Updated): 11/1/2022, 11:37:57 Vscode Extension Version: 1.73.1 on Debian Linux

Steps to reproduce:

Copy the following code into an editor with the extension activated:

/*
*! This is green
TODO? This is orange
*/
//*! This is red
//TODO? This is blue

Further details:

The preferred behavior, in my opinion, would be the behavior shown by multiline comments. Inline comments currently provide unexpected behavior, i.e. tags such as //TODO!, where a tag after another overwrites the first tag; while multiline comments provide more expected behavior, i.e. the first tag decides the comment styling.

aarondill commented 1 year ago

Taking a quick look through /src/parser.ts, it seems like this line in parser.SetRegex is the issue.

// parser.SetRegex;
this.expression += "(";
this.expression += characters.join("|");
this.expression += ")+(.*)";

The key issue I notice is the plus after the character list in SetRegex, matching all sequential tags, but only the first needs to be matched within the first group.

Using this function, you can see the difference with and without the operator.

function match(string, update) {
    let expression = "(\/\/)+( |\t)*"
    const characters=["\\*", "\\$"]
    expression += "(" + characters.join("|") + ")"
    if(!update) expression+="+"
    expression+="(.*)";
    const regexFlags = "ig";
    const regEx = new RegExp(expression, regexFlags);
    const match = regEx.exec(string);
    console.log(`regex used: /${expression}/${regexFlags}`)
    console.log(`tag found: ${match[3]}`)
    return match
}
match("//*$hello", false) //tag found: $
match("//*$hello", true) //tag found: *

as you can see, without the plus, it matches just the first tag that occurs, matching any other characters(including other tags) in the 4th capture group.

Unless there was a particular reason(perhaps language compatibility?) that I am unaware about, it seems this is a mistake and should be removed to allow expected behavior.