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

false issue created if comment matches identifier in a substring #234

Closed
rgalonso opened this issue Nov 11, 2024 · 4 comments
Closed

false issue created if comment matches identifier in a substring #234

rgalonso opened this issue Nov 11, 2024 · 4 comments

Comments

@rgalonso
Copy link
Contributor

The identifier matching is not done using word breaks. Rather, it looks for either a whitespace character or a colon to follow the identifier. i.e. TODO issue_title and TODO: issue_title will both match, but TODO at the end of the line will not. However, this doesn't consider the possibility that non-whitespace characters precede the identifier. In other words, this means that due to this pattern match and case-insensitivity, using FIX as an identifier leads to the following comment being matched

  suffixed_str = base + suffix # add the suffix after the base string

The fix of suffix is seen as an identifier and after the base string is considered the issue title.

Word breaks should be used for pattern matching the identifier. i.e. \b{identifier}\b

rgalonso added a commit to rgalonso/todo-to-issue-action that referenced this issue Nov 12, 2024
These tests capture the existing issues alstr#234 and
issue alstr#235. As no solution is in place yet, they're
marked as expected failures.
rgalonso added a commit to rgalonso/todo-to-issue-action that referenced this issue Nov 12, 2024
…ng of TODOs

Some parts of the code were using case-insensitive
matches when searching for an identifier (i.e.
TODO and todo would both be acceptable) whereas
other parts of the code would search for a strict
case-sensitive match (only TODO, not todo). This
inconsistency led to many issues, among them
- alstr#216
- alstr#224
- alstr#225

Further, the identifier match wasn't being done
using word breaks, meaning an identifier of "FIX"
would match (case-insensitively) with "suffix" or
"prefix" if those words happened to appear in a
comment. (See alstr#234).

Issue alstr#230 was also preventing issue labels from
being applied properly if the identifier case
did not match exactly the canonical version. i.e.
"todo" would generate an issue, but the associated
labels wouldn't be applied because the exact
identifier used wasn't "TODO".

This commit resolves all of these issues.
rgalonso added a commit to rgalonso/todo-to-issue-action that referenced this issue Nov 12, 2024
rgalonso added a commit to rgalonso/todo-to-issue-action that referenced this issue Nov 12, 2024
…ng of TODOs

Some parts of the code were using case-insensitive
matches when searching for an identifier (i.e.
TODO and todo would both be acceptable) whereas
other parts of the code would search for a strict
case-sensitive match (only TODO, not todo). This
inconsistency led to many issues, among them
- alstr#216
- alstr#224
- alstr#225

Further, the identifier match wasn't being done
using word breaks, meaning an identifier of "FIX"
would match (case-insensitively) with "suffix" or
"prefix" if those words happened to appear in a
comment. (See alstr#234).

Issue alstr#230 was also preventing issue labels from
being applied properly if the identifier case
did not match exactly the canonical version. i.e.
"todo" would generate an issue, but the associated
labels wouldn't be applied because the exact
identifier used wasn't "TODO".

This commit resolves all of these issues.
rgalonso added a commit to rgalonso/todo-to-issue-action that referenced this issue Nov 12, 2024
@alstr
Copy link
Owner

alstr commented Nov 12, 2024

I think this stems from the earliest version of the action, which didn't support other identifiers, where I must have been like "nothing can ever feature the word todo in". 😆

@rgalonso
Copy link
Contributor Author

rgalonso commented Nov 12, 2024 via email

@alstr
Copy link
Owner

alstr commented Nov 12, 2024

Yeah, I'm sure there are cases, word boundaries are definitely the way to go.

@rgalonso
Copy link
Contributor Author

rgalonso commented Dec 2, 2024

I think the changes of v5.1.2 (and then refined in v5.1.3) have closed this. Feel free to re-open if there's a corner case I haven't considered.

@rgalonso rgalonso closed this as completed Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants