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

"Identifier" search regexp fails for TOML front-matter #10

Closed
kaushalmodi opened this issue Jun 11, 2022 · 11 comments
Closed

"Identifier" search regexp fails for TOML front-matter #10

kaushalmodi opened this issue Jun 11, 2022 · 11 comments

Comments

@kaushalmodi
Copy link
Contributor

Hello,

I am afraid that this fails when using TOML front-matter:

denote/denote-link.el

Lines 103 to 104 in f91d247

(defconst denote-link--identifier-regexp "^\\(#\\+\\)?\\(identifier:\\)[\s\t]+\\(.*\\)"
"Regular expression for filename key and value.")

We will need to look for : or =, with optional space before =.

Sample TOML format output

+++
title      = "testtoml"
date       = 2022-06-11T11:49:10-04:00
tags       = ["test"]
identifier = "20220611T114910"
+++

I was actually looking into it to see if I can live with just the ID property when using Org front-matter (Ref #8).

Do you think this is the only thing we'll need to tweak if I want to have Org front-matter without the identifier keyword?

:PROPERTIES:
:ID: 20220611T114436
:END:
#+title:    test
#+date:     [2022-06-11 Sat 11:44]
#+filetags: test

Or do you plan to deeply bake in the reliance on #+identifier / identifier: / identifier = matches?

@protesilaos
Copy link
Owner

We will need to look for : or =, with optional space before =.

Oh, of course! Sorry about that. Will fix it right away.

Or do you plan to deeply bake in the reliance on #+identifier / identifier: / identifier = matches?

I think this is open to discussion, given that we still are at the point where we can make breaking changes.

@protesilaos
Copy link
Owner

The "^\\(#\\+\\)?\\(identifier\\)\\([\s\t]+\\)?[:=]\\([\s\t]+?\\)?\"?\\([0-9T]+\\)\"?" matches all of the following with M-x re-builder. Is there some other permutation we need to consider?

identifier = "20220611T114910"
identifier= 20220611T114910
identifier=20220611T114910
identifier= "20220611T114910"
identifier     ="20220611T114910"



identifier : "20220611T114910"
identifier: 20220611T114910
identifier:20220611T114910
identifier: "20220611T114910"
identifier     :"20220611T114910"

@kaushalmodi
Copy link
Contributor Author

Is there some other permutation we need to consider?

Nope this covers (more than) everything!


(more than): Few illegal cases like identifier= 20220611T114910 and identifier=20220611T114910 too. But that's OK.


Regexp suggestion:

"\\bidentifier\\s-*[:=]\\s-*\"?\\([0-9T]+\\)"

image

@protesilaos
Copy link
Owner

Regexp suggestion:

Vastly superior! Thanks!

I guess now we need to abstract the "indentifier" part, right? That gives the user maximum flexibility.

@kaushalmodi
Copy link
Contributor Author

Suggestion 2:

"^.?.?\\b\\(identifier\\|ID\\)\\s-*[:=]\\s-*\"?\\([0-9T]+\\)"

image

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented Jun 11, 2022

I think ^.?.?\\b will be good enough to avoid any false matches.

@protesilaos
Copy link
Owner

Very wel!! Since you wrote it, do you want to submit a PR (or send a patch)? If you are on mobile, I can commit with you as the author (if you want).

@kaushalmodi
Copy link
Contributor Author

I haven't tested it properly and won't have time to do so until tonight (~10 hrs from now).

But I can create a PR and let you decide. If the PR is still open after I am done testing, and I need to update it, I'll do so, else will create another PR

@protesilaos
Copy link
Owner

Okay, thank you! Will wait then.

kaushalmodi added a commit to kaushalmodi/denote that referenced this issue Jun 11, 2022
Fixes issue 10 over at the Github mirror:
<protesilaos#10>.

This regexp update now also matches the ID property. Refer issue 8 at
the Github mirror: <protesilaos#8>.
@kaushalmodi
Copy link
Contributor Author

Closing this as it was fixed by #11.

@protesilaos
Copy link
Owner

Thank you!

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

No branches or pull requests

2 participants