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

Improve keyword regex anchoring #28

Merged
merged 3 commits into from Oct 3, 2018

Conversation

Projects
None yet
2 participants
@wavexx
Copy link
Contributor

wavexx commented Jul 10, 2018

  • Anchor the keywords themselves, not the punctuation (fixes #27)
  • Match a variable amount of punctuation at the end (highlights correctly "TODO?!?" with hl-todo-highlight-punctuation ":!?")
@tarsius

This comment has been minimized.

Copy link
Owner

tarsius commented Oct 3, 2018

Sorry for the long delay.

At least in emacs-lisp-mode your change causes "TODO." to not be highlighted at all.

@tarsius

This comment has been minimized.

Copy link
Owner

tarsius commented Oct 3, 2018

Also, please do Match a variable amount of punctuation after the keyword in a separate commit.

@wavexx wavexx force-pushed the wavexx:fix_regex_anchor branch from 70c431c to 051a254 Oct 3, 2018

@wavexx

This comment has been minimized.

Copy link
Contributor Author

wavexx commented Oct 3, 2018

I didn't notice. Indeed, emacs-lisp has yet another definition of symbol characters.
I use a plain word delimiter now (< >), which seems to work as expected in elisp/python/c-modes.

As before, this assumes the keyword-faces are plain words. I would write this in the help of hl-todo-keyword-faces. Using "TODO:" as I was doing before wouldn't work.

@tarsius

This comment has been minimized.

Copy link
Owner

tarsius commented Oct 3, 2018

I use a plain word delimiter now (< >), which seems to work as expected in elisp/python/c-modes.

I can confirm that this works as expected. It also makes more sense.

There is one change in behavior, the TODO:noise in this testing file I used:

;;            character syntax meaning
;; TODO TODO: :         _      symbol
;; TODO TODO§ §         .      punctuation
;; TODO()
;; TODO(topic)  previously explicitly highlighted, now implicitly
;; TODO:noise   previously not highlighted, now highlighted
;; TODOnoise    still not highlighted

But that might actually be desirable. After all we previously made sure the TODO in TODO(topic), so doing the same for TODO.topic makes sense. As long as nothing in TODOnoise gets highlighted I am fine with this.

wavexx added some commits Oct 3, 2018

hl-todo--regexp: Match a variable amount of punctuation
Highlight correctly "TODO?!?" when `hl-todo-highlight-punctuation'
is e.g. ":!?".
hl-todo--regexp: Anchor the keywords themselves
Exclude the non-word characters from anchoring.

This was useful for testing:

;;            character syntax meaning
;; TODO TODO: :         _      symbol
;; TODO TODO§ §         .      punctuation
;; TODO()
;; TODO(topic)  previously explicitly highlighted, now implicitly
;; TODO:noise   previously not highlighted, now highlighted
;; TODOnoise    still not highlighted

Fixes #27.
See #28.

@tarsius tarsius force-pushed the wavexx:fix_regex_anchor branch from 051a254 to 445d78a Oct 3, 2018

@tarsius

This comment has been minimized.

Copy link
Owner

tarsius commented Oct 3, 2018

I reverted

-        (concat "\\<\\("
+        (concat "\\(\\<"

as that makes no difference and would result in < ( > ), and since I like s-expressions I cannot deal with that.

I changed the commit messages a bit and added additional information. Please have another look before I merge.

@wavexx

This comment has been minimized.

Copy link
Contributor Author

wavexx commented Oct 3, 2018

@tarsius

This comment has been minimized.

Copy link
Owner

tarsius commented Oct 3, 2018

Because \<...\> and \(...\) serve completely different purposes (and \< matches the empty string) it doesn't actually make any difference whether we write \\<\\( or \\(\\<. However because we have to keep things well balanced when writing e.g. s-expressions or html (([)] and <p><b></p></b> are invalid), we might as well do the same here.

The previous order is better in a superficial way. If this code were written from scratch and you used the "wrong" order, then that wouldn't make a difference and I might not have noticed. However there is absolutely no reason to go from the "better" to the "worse" variant, when that doesn't even make a difference. The change is not only superficially worse though, it is also worse because it is an unnecessary change, i.e. it results in a more complicated diff than necessary.

@tarsius tarsius merged commit c759967 into tarsius:master Oct 3, 2018

@wavexx

This comment has been minimized.

Copy link
Contributor Author

wavexx commented Oct 3, 2018

@tarsius

This comment has been minimized.

Copy link
Owner

tarsius commented Oct 3, 2018

Ups.
Pushed another commit to fix that.

@wavexx

This comment has been minimized.

Copy link
Contributor Author

wavexx commented Oct 3, 2018

@tarsius

This comment has been minimized.

Copy link
Owner

tarsius commented Oct 3, 2018

oO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.