Replace title and table sregexp to rx samples #31
Conversation
adoc-mode.el
Outdated
| (setq-local comment-end "") | ||
| (setq-local comment-start-skip "^//[ \t]*") | ||
| (setq-local comment-end-skip "[ \t]*\\(?:\n\\|\\'\\)") | ||
| (set (make-local-variable 'comment-column) 0) |
There was a problem hiding this comment.
The previous code does exactly the same, but more compactly. :-)
adoc-mode.el
Outdated
| @@ -1022,8 +1093,8 @@ LEVEL starts at 1." | |||
|
|
|||
| (defun adoc-make-two-line-title-underline (level &optional length) | |||
| "Returns a two line title underline. | |||
| LEVEL is the level of the title, starting at 1. LENGTH is the | |||
| line of the title's text. When nil it defaults to 4." | |||
| LEVEL is the level of the title, starting at 1. LENGTH is the | |||
There was a problem hiding this comment.
This indentation seems wrong to me.
adoc-mode.el
Outdated
| ((adoc-table-regexp) 0 ,adoc-table-face) | ||
| ((adoc-code-regexp) 0 ,adoc-code-face)) | ||
| "Font lock keywords used to highlight text in AsciiDoc files mode." | ||
| :version "26.1" |
There was a problem hiding this comment.
That's the version of adoc-mode, not Emacs.
| @@ -3416,16 +3487,32 @@ LOCAL-ATTRIBUTE-FACE-ALIST before it is looked up in | |||
| "Keymap used in adoc mode.") | |||
|
|
|||
|
|
|||
| (defcustom adoc-mode-font-lock-keywords | |||
There was a problem hiding this comment.
Why does this have the same name as the variable that you set above? Seems incorrect to me.
There was a problem hiding this comment.
Variable adoc-mode-font-lock-keywords is used with (setq adoc-mode-font-lock-keywords...) but not defined. I don't see code like this:
(defvar adoc-mode-font-lock-keywords ...)I used example from dns-mode.el for writing this code. Also I saw some TODO's like this:
1539 ;; TODO: use same regexps as for font lock
I think, it is a good idea replace functions for regexp to constants with more human readable (and maintain!) code, based on rx.
There was a problem hiding this comment.
I think you didn't get my point - this defcustom has some value that simple gets overwritten here:
(setq adoc-mode-font-lock-keywords (adoc-get-font-lock-keywords))
I'm fine with simplifying code by using rx, but I just don't understand what's the point of this defcustom right now.
There was a problem hiding this comment.
Maybe, I add before adoc-calc function declaration like this?
(defvar adoc-font-lock-keywords nil "Keywords for font-lock in `adoc-mode'.")There was a problem hiding this comment.
I'd probably remove this adoc-calc completely, as few modes do something like this. If you want to factor in some font-lock customizations you typically just disable/enable the mode and its init code will take care of such updates. In general it's very bad style to have a top-level function call to initialize anything (as adoc-mode currently does).
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## master (unreleased) | |||
|
|
|||
| ## 0.7.1 (2023-04-05) | |||
There was a problem hiding this comment.
No need for this release header, as it will be added on release time. The heading here should actually be "Bugs fixed".
| @@ -2676,7 +2747,7 @@ new customization demands." | |||
| (null adoc-unichar-alist)) | |||
| (adoc-make-unichar-alist)) | |||
|
|
|||
| (setq adoc-font-lock-keywords (adoc-get-font-lock-keywords)) | |||
| (setq adoc-mode-font-lock-keywords (adoc-get-font-lock-keywords)) | |||
There was a problem hiding this comment.
As you can see we intentionally don't use variables named adoc-mode-something most of the time, so we can have shorter identifiers. Please, don't rename this variable.
|
You should also check the CI error - seems some |
|
Adding a few unit tests covering your changes would be nice as well. |
|
Perhaps it is worth closing this PR and opening it again after fixing all issues. Should I remove the 'adoc-calc` function? |
Up to you - I don't mind keeping it open and marked as draft.
Yep. |
| eol))) | ||
| "Regular expression for AsciiDoc table.") | ||
|
|
||
| (defconst adoc-code-regexp |
There was a problem hiding this comment.
Side-note: Without source-style these are actually listings, see https://docs.asciidoctor.org/asciidoc/latest/blocks/delimited/.
| (rx (seq | ||
| (minimal-match | ||
| bol | ||
| "----" |
There was a problem hiding this comment.
This rigid kind of regexp is wrong.
The fence is actually "----+" and the end fence must exactly match the start fence.
This makes nested blocks possible.
See https://docs.asciidoctor.org/asciidoc/latest/blocks/delimited/#linewise-delimiters.
rxbuiltin library.rx.rx.dns-mode.el).close #24