-
-
Notifications
You must be signed in to change notification settings - Fork 275
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
Avoid regex if possible checking for \AssignTemplateKeys #1631
Conversation
@Skillmon Take a look? |
base/lttemplates.dtx
Outdated
@@ -1400,8 +1401,15 @@ | |||
\@@_if_keys_exist:nnT {#1} {#2} | |||
{ | |||
\@@_store_key_implementation:nnn {#1} {#2} {#4} | |||
\regex_match:nnTF { \c { AssignTemplateKeys } } {#5} | |||
{ \@@_declare_template_code:nnnn {#1} {#2} {#3} {#5} } | |||
\str_if_in:nnTF {#5} { AssignTemplateKeys } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less likely false-positives (marginally, I admit):
\str_if_in:nnTF {#5} { AssignTemplateKeys } | |
\str_if_in:nnTF {#5} { \AssignTemplateKeys } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd thought about this - I missed out the backslash just in case we are in a context where it's not the escape char. I agree it's very much an edge case.
base/lttemplates.dtx
Outdated
\@@_declare_template_code:nnnn | ||
{#1} {#2} {#3} { \AssignTemplateKeys #5 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether this is desirable, but one could reduce code duplication here using the following code
\str_if_in:nnT {#5} { \AssignTemplateKeys }
{
\regex_match:nnT { \c { AssignTemplateKeys } } {#5}
{
\@@_declare_template_code:nnnn {#1} {#2} {#3} {#5}
\prg_break:
}
}
\@@_declare_template_code:nnnn
{#1} {#2} {#3} { \AssignTemplateKeys #5 }
\prg_break_point:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would \AssignTemplateKeys inside a group in the code of a template make any sense? If not, then this should perhaps be documented and you could go \tl_if_in...
but I guess in reality any of the tests are fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have one case in a group: enotez
- it though uses \group_begin:
/\group_end:
. We could document this and make a formally breaking change - do we want to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just wondering if it would make sense at all. If in a group the assignments would get reverted at the end of the group wouldn't they? so what happens then in enotez?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason we kept \AssignTemplateKeys
in the first place was that you might want to either save/restore variables or (as in enotez
) use a group to keep changes to each use case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok then there are use cases for using \AssignTemplateKeys in a group, so we can't check using \tl_if_in...
, so let's forget it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well expl3
groups really should be \group_begin: ... \group_end:
not {..}
, but it's hard to be 100% sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arguably yes, but then as you say that is a bit hard to enforce and it is not critically slower to use a str test here (given that this happens only during declarations). So I think we are good.
a9746b4
to
9e537e1
Compare
READ ME FIRST: Please understand that in most cases we will not be able to merge a pull request because there are a lot of internal activities needed when updating the LaTeX2e sources. If you have a code suggestion please discuss it with the team first.
Internal housekeeping
Status of pull request
Checklist of required changes before merge will be approved
\changes
entries in source includedchanges.txt
updatedltnewsX.tex
(and/orlatexchanges.tex
) updated