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

Src block/33font lock fontified missing #37

Merged
merged 8 commits into from
Aug 3, 2023

Conversation

TobiasZawada
Copy link
Collaborator

  • Introduce adoc-kwf-search that skips code blocks
  • If a match is prevented in adoc-kwf-std, re-start search at the match string -- not at last start + 1 (this speeds up font-locking)
  • re-structure adoc-kw-replacement such that it has the same speed-up as adoc-kwf-std and the code for the search operation is simplified
  • add an ert test for adoc-kw-replacement

Tobias Zawada and others added 6 commits July 24, 2023 22:09
- Set text property `font-lock-fontified` for fontified code blocks
- also fix compiler error by quoting lambdas with sharp quote
Replace `re-search-forward` by `adoc-kwf-search` in font-lock keywords.
`adoc-kwf-search` continues search if the adoc-code-block text property
is set at the matching position.
Also add ert test for keyword-replacement
adoc-mode.el Outdated
Like `re-search-forward' with the same arguments
REGEXP, BOUND, NOERROR and COUNT.
If a match for REGEXP is found where the text property
`adoc-code-block' is non-nil continue the search."
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably you should also mention explicitly that the purpose of this is skip code in code blocks for performance reasons.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not only for performance reasons but also to prevent the adoc fontification within code blocks.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this should be mentioned in the docstring as well then.

@@ -1556,6 +1556,19 @@ Subgroups of returned regexp:


;;;; font lock keywords
(defsubst adoc-kwf-search (regexp &optional bound noerror count)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why kwf and not just kw?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because adoc-kwf-search does not return a font-lock keyword but it is an auxiliary Function much like adoc-kwf-std.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get this, but I'm struggling to understand what does kwf stands for. :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that -kwf- stands for keyword function.
Those are functions that are used in the keyword generators -kw-.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, here it's not even a function, as I see it's being inlined. :-) Anyways, I don't feel strongly about this naming - I just find it somewhat confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasoning for the naming:

  1. Keep changes small
  2. Keep consistency

@bbatsov
Copy link
Owner

bbatsov commented Aug 2, 2023

One final comment - I find it weird that the first commit is marked as fixing #33 and all commits afterwards are marked as addressing it.

I'd suggested that the main fix be marked with [Fix #33] ... and things that are related should be just have [#33] ... as their prefix. That's a much more common way to name commits and for a good reason - it's easier to read to figure out what is related to what.

@TobiasZawada
Copy link
Collaborator Author

TobiasZawada commented Aug 2, 2023

I'd suggested that the main fix be marked with [Fix #33] ... and things that are related should be just have [#33] ... as their prefix. That's a much more common way to name commits and for a good reason - it's easier to read to figure out what is related to what.

Side-note: The "Addresses"/"Fixes"-Style is adopted from a project with about 10k C++-source/header-files. It contains many more files in other programming languages. The .git-database of the Git-repo alone is about 20GB. The style was introduced several years ago by a professional software developer.

But, I am adopting to your suggested style. Let's hope that I do remember and take care in the comments for the next pull-request.

@TobiasZawada
Copy link
Collaborator Author

@bbatsov Let Lee Reed no longer wait for the fix of #33.

@TobiasZawada TobiasZawada merged commit 49bbd9a into master Aug 3, 2023
8 checks passed
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 this pull request may close these issues.

2 participants