Shuo Shen <shuoshen...@gmail.com> writes:

> This is the first patch for the feature request: [FR] Allow emojis in
> tags (was: emojis in tags?)
> (https://list.orgmode.org/orgmode/87wn4pdarh.fsf@localhost/)

(Note that the exact changes needed to support emoji or other things may
need to be discussed first.)

> It's my first diff. I'm trying to get an understanding of code as well
> as the process at the same time, and don't want to get things
> complicated. Therefore, I'm keeping the change minimal.

Thanks! See my comments below.

> Subject: [PATCH] lisp/org.el: Define a const for the tag matching pattern
>
> * org.el (org-tag-valid-char-set, org-tag-re, org-tag-group-re)
> (org-tag-line-re): Defined the const org-tag-valid-char-set.

In the commit messages we quote symbols like `org-tag-valid-char-set'.

> org-tag-valid-char-set will be used as a single source of truth for
> matching tags. The problem is that the tag matching pattern is defined

... and use double space between sentences.
See https://orgmode.org/worg/org-contribute.html#org219e2af

> everywhere (org-tag-re, org-tag-group-re, org-tag-line-re, and a bunch
> of inline places. This is the first diff to address the problem. When
> the refactorizations are all done, we will add emoji support for tags

I am not sure if we need to be so granular in constants. Maybe we can
instead just have `org-tag-re' and derive other regexps from it?

> +(defconst org-tag-valid-char-set "[:alnum:]_@#%"
> +  "[Tag] A string representing the set of characters and character
> + classes valid within a tag.  This is the base definition for tag
> +characters and will be the place to add new patterns.")

We usually do not announce future plans in docstrings. Docstring are for
the users to read, and announcing changes in Org syntax that may or may
not happen is not a good idea.

> +(defconst org-tag-re (format "[%s]+" org-tag-valid-char-set)
>    "Regexp matching a single tag.")

Since we are rewriting things here, may as well use rx for better readability.
  
> +(defconst org-tag-group-re
> ...
>  (defconst org-tag-line-re
> -  "^\\*+ \\(?:.*[ \t]\\)?\\(:\\([[:alnum:]_@#%:]+\\):\\)[ \t]*$"
> +  (format "^\\*+ \\(?:.*[ \t]\\)?\\(:\\([%s:]+\\):\\)[ \t]*$" 
> org-tag-valid-char-set)

Note that tag regexp is used manually in a number of places in Org
codebase. Search for "_@#%:" across Org codebase and see. All these
instances should be changed and ideally use the regexp constants instead
of hard-coded constants.

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Reply via email to