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

repeat of TODO comment line inserted instead of issue URL #224

Closed
rgalonso opened this issue Nov 8, 2024 · 2 comments · Fixed by #228
Closed

repeat of TODO comment line inserted instead of issue URL #224

rgalonso opened this issue Nov 8, 2024 · 2 comments · Fixed by #228

Comments

@rgalonso
Copy link
Contributor

rgalonso commented Nov 8, 2024

If using non-standard capitalization of an identifier (e.g. "ToDo" instead of the standard "TODO"), instead of inserting the issue URL as a comment after the TODO, a verbatim copy of the TODO comment line is inserted.

When searching for an identifier (e.g. TODO, FIXME), a case-insensitive search is performed. e.g. This will find "todo" in addition to "TODO". That's OK, because different people prefer different approaches to that1. However, when it comes time to search for that line in the code (in order to insert the issue URL comment), the search is case-sensitive. This leads to the behavior noted in the first paragraph.

Footnotes

  1. We might want to consider making a case-sensitive match an option though. There could be projects where they strictly want to match "TODO" but not "todo", for instance.

@rgalonso
Copy link
Contributor Author

rgalonso commented Nov 8, 2024

@alstr, I've already got a solution for this. I just wanted to formally make an issue for this so we have traceability for all the changes I'm getting ready to push.

@alstr
Copy link
Owner

alstr commented Nov 8, 2024

@alstr, I've already got a solution for this. I just wanted to formally make an issue for this so we have traceability for all the changes I'm getting ready to push.

Thanks!

rgalonso added a commit to rgalonso/todo-to-issue-action that referenced this issue Nov 9, 2024
There are issues currently where the wrong text
may be inserted in place of the issue URL. (See
github.com/alstr/todo-to-issue-action issues alstr#224
, alstr#225, and alstr#226, for example.) When such a
situation is detected, the offending line is now
NOT inserted and instead a warning message is
printed out. This doesn't completely resolve
those issues, but mitigates against them and
other similar issues which may arise in the future.
rgalonso added a commit to rgalonso/todo-to-issue-action that referenced this issue Nov 9, 2024
When searching for the comment line after which
the issue URL will be inserted, use a case-insensitive
search for the identifier portion of the line.
This matches how TodoParser identifies TODOs using
a case-insensitive search.

Closes github.com/alstr#224
rgalonso added a commit to rgalonso/todo-to-issue-action that referenced this issue Nov 10, 2024
Added tests for issues alstr#224/alstr#225 and alstr#229. Since
these are known bugs, the tests are currently
marked as expected failures. Once the solution
is implemented, that designation can be removed
and we'll still keep the test to make sure there
aren't any regressions.
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
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

Successfully merging a pull request may close this issue.

2 participants