Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: task tags are ignored if not at the start of a comment #532

Merged
merged 1 commit into from
May 16, 2023

Conversation

sebthom
Copy link
Member

@sebthom sebthom commented May 15, 2023

as of the last release task tags are only evaluated if they are at the beginning of a comment.
this PR restores the previous behaviour of evaluating task tags anywhere in the comment.

@sebthom sebthom changed the title revert task tag regex change fix: revert task tag regex change May 15, 2023
@github-actions github-actions bot added the bug label May 15, 2023
@sebthom sebthom changed the title fix: revert task tag regex change fix: task tags are ignored if not at the start of a comment May 15, 2023
@sebthom sebthom force-pushed the task-tags-regex branch 2 times, most recently from e37aebb to b4b3565 Compare May 15, 2023 20:57
@@ -50,7 +50,7 @@ public static void reloadMarkerConfigs() {
MARKERCONFIG_BY_TAG.put(markerConfig.tag, markerConfig);
}
TAG_SELECTOR_PATTERN = Pattern
.compile("^\\s*(" + MARKERCONFIG_BY_TAG.keySet().stream().collect(Collectors.joining("|")) + ")\\b");
.compile("\\b*(" + MARKERCONFIG_BY_TAG.keySet().stream().collect(Collectors.joining("|")) + ")\\b");
Copy link
Contributor

@rubenporras rubenporras May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sebthom ,

If we remove the "^", I think we not need the "*", I think \b should be sufficient, instead of \b*.

I have a bit more time this morning, and I think that the problem is not the regex, but the fact that the comment delimiter is part of the comment string, commentText in my case contains "-- TODO this and that", and not " TODO this and that". So ^\\s* will never match any comment.

Is that maybe the problem? A bit above there is a check

					if (commentText.length() < 3)
						continue;

I am not sure how it all fits together but if the comment separator is going to be part of the string, that condition will almost always be true, so one could as well remove it and rely on the matcher, I do not think performance wise it is much of a speedup, since in this case most comments will pass that condition. In addition there is no hint that task patterns must have more than n characters.

Sorry for the bunch of unstructured information/questions. I do not know the code at all, so I am just commenting just in case you have overlooked something. If not, you can ignore my comments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove the "^", I think we not need the "*", I think \b should be sufficient, instead of \b*.

That is correct. I changed it. Thanks!

I have a bit more time this morning, and I think that the problem is not the regex, but the fact that the comment delimiter is part of the comment string, commentText in my case contains "-- TODO this and that", and not " TODO this and that". So ^\\s* will never match any comment.

Interesting. In the languages that I am using/testing (Dart and Haxe), the commentText does not contain the comment delimiter. Maybe it depends on how the textmate rule is defined.

Is that maybe the problem? A bit above there is a check

					if (commentText.length() < 3)
						continue;

I am not sure how it all fits together but if the comment separator is going to be part of the string, that condition will almost always be true, so one could as well remove it and rely on the matcher, I do not think performance wise it is much of a speedup, since in this case most comments will pass that condition. In addition there is no hint that task patterns must have more than n characters.

I am hitting that condition quite often in Dart and Haxe so I would prefer to keep that check.

@rubenporras
Copy link
Contributor

BTW: Our single line comment is defined as

        "linecomment": {
            "name": "comment.line.double-dash.avqscript",
             "begin": "(^[ \\t]+)?(?=--)",
             "beginCaptures": {
               "1": {
                 "name": "punctuation.whitespace.comment.leading.ts"
               }
             },
            "end": "(?=$)"
        },

Then if the line has the text " -- TODO this and that" then the variable commentText has first the content " ", and the token is "comment.line.double-dash.avqscript-punctuation.whitespace.comment.leading.ts" and then "-- TODO this and that" nd the token is "comment.line.double-dash.avqscript".

Copy link
Contributor

@rubenporras rubenporras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me now. IIRC, it is the same regex that we had before the last release

@sebthom
Copy link
Member Author

sebthom commented May 16, 2023

This looks good to me now. IIRC, it is the same regex that we had before the last release

That was actually my intention with this PR but I apparently needed some attempts...

@sebthom sebthom merged commit d2fa7d3 into eclipse:main May 16, 2023
@sebthom sebthom deleted the task-tags-regex branch May 16, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants