Improvements #75
Improvements #75
Conversation
minad
force-pushed
the
improvements
branch
6 times, most recently
from
e2663fa
to
126c4d1
Compare
Scan the whole buffer since Flymake does not move other diagnostics outside the changed region. Flymake is executed in an idle timer so performance should be acceptable. If not, we could try to cache the locations.
Use the whole line as diagnostic text if the keyword is not directly behind a comment. This heuristic gives a good result for the common cases. ;; TODO: Some task -> "TODO: Some task" ;; Text with TODO keyword -> "Text with TODO keyword"
This moves in the opposite direction from c139f11. That didn't provide much justification, but at least it provided some. Why do you think your "simplified setup" is better? |
Well it is simpler, which can be considered better. Furthermore we allocate the list only once. I wasn't aware that you wanted to enable additional customization possibilities. (EDIT: I reverted this commit, or rather removed it via force pushing.) |
Avoid costly allocation during iteration.
Optimization; Access the variable hl-todo--regexp only via the corresponding function, which ensures that the variable is initialized.
|
Okay, I believe I addressed all your remarks. Please take another look. |
|
Thanks, I have merged that now. Sorry for the passive-aggressive review. I didn't sleep well and woke up in a bad mood, because of the heat and the construction site in front of my house.
If I remember correctly, some users want to use different keywords in different buffers, and I think we better preserve that option. |
|
Thanks! No worries. It is all good. I usually try to avoid making superfluous changes to existing code, so you were right to point that out.
This shouldn't matter since the font lock keywords do not explicitly contain the regexp. It is all indirect via From my understanding, using a buffer local font lock variable doesn't have real advantages. If one wants to tweak the value, then one could still turn the global variable into a local one via |
Right. All this really does is inline the setup code. I have resurrected your commit, changing the commit message to focus on that aspect. Does 342ee27 still look right to you? |
|
On 8/28/23 21:56, Jonas Bernoulli wrote:
This shouldn't matter since the font lock keywords do not explicitly
contain the regexp. It is all indirect via |hl-todo--search| and
|hl-todo--get-face|. The only thing that matters is that the regexp
and the keywords itself can be set locally.
Right. All this really does is inline the setup code. I have resurrected
your commit, changing the commit message to focus on that aspect.
Inlining is of course a valid simplification. But I didn't only inline
the `hl-todo--setup` function. Additionally I had made the
`hl-todo--keywords` variable global and initialized it once globally
instead of per-buffer inside the minor mode body.
I think my wording above was misleading. By "regexp and keywords" I
meant the todo-keywords like "FIXME", "TODO", etc. and not the font
locking keywords. As I had said before, having a buffer-local
`hl-todo--keywords` variable doesn't seem advantageous to me.
If users want to configure todo-keywords per buffer, they only have to
make `hl-todo-keyword-faces` buffer-local, adjust its local value as
desired, and reset `hl-todo--regexp` to nil, such that the cached regexp
is reinitialized.
|
But you didn't do that in that commit, did you? Maybe it would be best if you opened a new pull-request with all the changes that you still think are beneficial but which you removed after my initial feedback. |
Hi Jonas,
I discovered some issue with the flymake checker and made some other minor improvements on the way (doc strings, linting and optimizations). I hope you like the changes. Thanks!