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: Inconsistent behavior between hl-todos and magit-todos #125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elfsternberg
Copy link

This patch addressses Magit-todos Issue 124.

Magit-todos has inconsistent behavior with hl-todos. If a keyword doesn't
start with a whitespace character (using the elisp-regexp symbol blank),
hl-todos will find it, but magit-todos will not.

This has consequences for documentation writers who typically leave their
todo tasks in-line with the documentation, using the "to come" convention
of [TK: Do something here]. Since the keyword "TK" does not have a leading
space, hl-todos will find it, but magit-todos will not.

This patch replaces the requirement that the keyword be proceeded with a
whitespace character with a more general case of (not alphanumeric). This
creates behavior that matches hl-todos more closely. This change is only
applied to the "non-org" branch of the macro.

This patch addressses [Magit-todos Issue 124](alphapapa#124).

Magit-todos has inconsistent behavior with hl-todos.  If a keyword doesn't
start with a whitespace character (using the elisp-regexp symbol `blank`),
hl-todos will find it, but magit-todos will not.

This has consequences for documentation writers who typically leave their
todo tasks in-line with the documentation, using the "to come" convention
of `[TK: Do something here]`. Since the keyword "TK" does not have a leading
space, hl-todos will find it, but magit-todos will not.

This patch replaces the requirement that the keyword be proceeded with a
whitespace character with a more general case of `(not alphanumeric)`. This
creates behavior that matches hl-todos more closely.  This change is only
applied to the "non-org" branch of the macro.
@alphapapa
Copy link
Owner

alphapapa commented Mar 26, 2022

I'm not sure if this qualifies as a fix or a change. I don't think I necessarily intended that magit-todos behave exactly the same as hl-todo in this regard; the use of hl-todo in this package is mostly for convenience regarding display faces.

Anyway, if this is helpful, I don't mind making this change. But it should probably be done with a customizeable variable, similar to magit-todos-keyword-suffix. Regarding that, see also #69 and #118.

What do you think? Thanks.

@tlotze
Copy link

tlotze commented Sep 2, 2022

FWIW, I would appreciate the change even though I'm not using the documentation writers' conventions. As a case in point, i stumbled upon this issue because of comment lines like #TODO (without a space) or # pragmatic choice/TODO. Granted, these may not be following conventions and it might be argued in this specific example that # TODO: reconsider whether this is acceptable as a pragmatic choice would be preferable, but these things tend to come naturally when jotting down a quick aside while keeping focused on something else. And when in doubt, I'd prefer the TODOs section to err on the side of containing too much, particularly with capitalised keywords that aren't likely to occur by coincidence. Otherwise, I expect to keep searching for those keywords by other means because I could never be sure whether some of them escape me because of punctuation.

@tlotze
Copy link

tlotze commented Sep 3, 2022

See the second commit of PR #136: The same logic that fixes #135 can be applied to the beginning of the keyword, thereby solving the present issue in a way that doesn't conflict with a possible addition of a customizable prefix regex as discussed in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants