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

Set template keys docu #1614

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Set template keys docu #1614

wants to merge 6 commits into from

Conversation

FrankMittelbach
Copy link
Member

Internal housekeeping

Status of pull request

  • Feedback wanted
  • Ready to merge

Checklist of required changes before merge will be approved

  • [n/a] Test file(s) added
  • [n/a] Version and date string updated in changed source files
  • [n/a] Relevant \changes entries in source included
  • [n/a] Relevant changes.txt updated
  • [n/a] Rollback provided (if necessary)?
  • [n/a] ltnewsX.tex (and/or latexchanges.tex) updated

One thing to consider: change name from \SetTemplateKeys to \OverwriteTemplateKeys, which seems to me sits better with \AssignTemplateKeys(as it is theny clear what the order would be). It is not used in texmf (other than in latex-lab) so that could be a breaking change without problems.

@FrankMittelbach
Copy link
Member Author

more exactly there is one usage in keytheorems by @mbertucci47 and I think we can arrange a smooth transition if we decide on a name change.

@josephwright
Copy link
Member

\SetTemplateKeys was modelled ultimately on \setkeys (which is why we have \keys_set:nn, etc.) - but if the overall feeling is an alternative name is better, I see no problem changing. If we do, there needs to be an adjustment in xtemplate (which is 'frozen' so does need to keep the existing name available).

base/lttemplates.dtx Outdated Show resolved Hide resolved
base/lttemplates.dtx Outdated Show resolved Hide resolved
@FrankMittelbach
Copy link
Member Author

\SetTemplateKeys was modelled ultimately on \setkeys (which is why we have \keys_set:nn, etc.) - but if the overall feeling is an alternative name is better, I see no problem changing. If we do, there needs to be an adjustment in xtemplate (which is 'frozen' so does need to keep the existing name available).

Maybe it should be called \SetKnownTemplateKeys then, because that's what it is and then a confusion with \AssignTemplateKeysis less likely (imo).

@josephwright
Copy link
Member

Maybe it should be called \SetKnownTemplateKeys then, because that's what it is and then a confusion with \AssignTemplateKeysis less likely (imo).

I thought that the fact this says 'template keys' suggested it would only handle known ones - but again no objection to a change.

@FrankMittelbach
Copy link
Member Author

Maybe it should be called \SetKnownTemplateKeys then, because that's what it is and then a confusion with \AssignTemplateKeysis less likely (imo).

I thought that the fact this says 'template keys' suggested it would only handle known ones - but again no objection to a change.

why would it? It could be a wrapper for \keys_set:nn as you wrote (and the name currently implies) and that would complain about unkown keys, wouldn't it?

@u-fischer
Copy link
Member

I don't quite like \OverwriteTemplateKeys. It doesn't clarify for me that the command should be used inside a template better than \SetTemplateKeys. Actually "overwrite" sounds for me more like a global forced change and so is more confusing than the \Set-command.
\SetKnownTemplateKeys is an option, but it mainly tells me that it won't error if unknown keys are passed to the command.

@FrankMittelbach
Copy link
Member Author

I don't quite like \OverwriteTemplateKeys. It doesn't clarify for me that the command should be used inside a template better than \SetTemplateKeys. Actually "overwrite" sounds for me more like a global forced change and so is more confusing than the \Set-command. \SetKnownTemplateKeys is an option, but it mainly tells me that it won't error if unknown keys are passed to the command.

Nothing really tells that it should only be used in the template code and it would be hard to get that into the name (which is why that is explicitly documented now) but using "Known" helps when reading code where it is in the right place, then you immediately see why it could get keys that do not fit for that particular template without ding harm and it fits with the underlying command which is \keys_set_known:nnN. But I'm not insisting on a change, just think that it might be result in a slightly better code reading.

@u-fischer
Copy link
Member

but using "Known" helps when reading code where it is in the right place

yes how unknown keys are handled is certainly a question that would come to my mind, and it would fit better the definition as the underlying command is \keys_set_known:nnN. Side question: should the list of unknown, unprocessed keys be made publicitly available somehow?

@FrankMittelbach
Copy link
Member Author

but using "Known" helps when reading code where it is in the right place

yes how unknown keys are handled is certainly a question that would come to my mind, and it would fit better the definition as the underlying command is \keys_set_known:nnN. Side question: should the list of unknown, unprocessed keys be made publicitly available somehow?

It would be nice, but rather hard, if even possible, to implement. For example, the key/vals to \begin{enumerate} are passed on to 3 or 4 templates making up the list, but also to the item template for the individual items in the list. Such scenarios make it imho infeasable to try to keep track of this. But if anybody sees a good way ...?

@FrankMittelbach
Copy link
Member Author

I have now updated and extended the documentation as well as moved the empty test inside the command, so I think it is ready for another review. What remains open is the csname. Are we good with \SetKnownTemplateKeys or should we keep the old name?

@u-fischer
Copy link
Member

It would be nice, but rather hard, if even possible, to implement. For example, the key/vals to \begin{enumerate} are passed on to 3 or 4 templates making up the list, but also to the item template for the individual items in the list. Such scenarios make it imho infeasable to try to keep track of this. But if anybody sees a good way ...

I didn't mean a general tracking. \SetTemplateKeys does a \keys_set_known:nnN { template / #1 / #2 } {#3} \l_@@_tmp_clist, and I wondered if this could be a public clist instead of an internal one, e.g. for checks in cases one knows that only known keys should be passed or if one want to pass the rest to an internal template. Or perhaps \SetKnownTemplateKeys could use four argument and the last one stores the remaining keys while \SetTemplateKeys stays at is?

@FrankMittelbach
Copy link
Member Author

It would be nice, but rather hard, if even possible, to implement. For example, the key/vals to \begin{enumerate} are passed on to 3 or 4 templates making up the list, but also to the item template for the individual items in the list. Such scenarios make it imho infeasable to try to keep track of this. But if anybody sees a good way ...

I didn't mean a general tracking. \SetTemplateKeys does a \keys_set_known:nnN { template / #1 / #2 } {#3} \l_@@_tmp_clist, and I wondered if this could be a public clist instead of an internal one, e.g. for checks in cases one knows that only known keys should be passed or if one want to pass the rest to an internal template. Or perhaps \SetKnownTemplateKeys could use four argument and the last one stores the remaining keys while \SetTemplateKeys stays at is?

ok, back to the drawing board then. what about this?

\ExplSyntaxOn

% error on unknown keys
\cs_set_protected:Npn \SetTemplateKeys #1#2#3
  {
    \tl_if_empty:oF {#3}
      {
        \keys_set:no { template / #1 / #2 } {#3}
      }
  }
  
 % supports unknown keys and returns them for further processing 
\cs_new_protected:Npn \SetKnownTemplateKeys #1#2#3#4
  {
    \tl_if_empty:oF {#3}
      {
        \keys_set_known:noN { template / #1 / #2 } {#3} #4
      }
  }

\SetKnownTemplateKeys {block}{display} {a=a, b=b, c=c} \foo
\show\foo
\SetKnownTemplateKeys {list}{display} {\foo} \bar
\show\bar

% process remaining keys (should be empty after that
% Or to be used when you know that you shouldn't have unknown ones
\SetTemplateKeys {para}{std} {\bar}

It would be for sure a breaking change, but I guess not a bad one.

@u-fischer
Copy link
Member

one would have to check the block code and adapt the uses there but beside this this looks fine imho.

@FrankMittelbach
Copy link
Member Author

one would have to check the block code and adapt the uses there but beside this this looks fine imho.

sure, the block code is already adapted as part of the PR (but it needs a corresponding change then)

@mbertucci47
Copy link
Contributor

@FrankMittelbach I've already removed the usage in keytheorems and use \EditInstance instead

@FrankMittelbach
Copy link
Member Author

@FrankMittelbach I've already removed the usage in keytheorems and use \EditInstance instead

ah, so much the better, then there is no usage of the current command other than in latex-lab, makes life easy. Btw, what to you think of the new docs and the suggested change/extension?

@mbertucci47
Copy link
Contributor

I like the documentation changes. I'm not sure I fully understand the discussion about known/unknown keys, but I'm not a fan of the lack of error when an unknown key is passed to, say, \begin{enumerate}. It's too easy to misremember leftmargin as left-margin or left margin and be confused why nothing is changing.

@josephwright
Copy link
Member

I like the documentation changes. I'm not sure I fully understand the discussion about known/unknown keys, but I'm not a fan of the lack of error when an unknown key is passed to, say, \begin{enumerate}. It's too easy to misremember leftmargin as left-margin or left margin and be confused why nothing is changing.

The problem is that as @FrankMittelbach has indicated, you might need several templates for a complex case like this. So how do you decide where to issue a warning? We really don't want users to have to work out which template is which for setting keys - we looked at prefixing all keys, and it doesn't make a good interface.

@Skillmon
Copy link
Contributor

Skillmon commented Jan 8, 2025

Nothing really tells that it should only be used in the template code and it would be hard to get that into the name (which is why that is explicitly documented now) but using "Known" helps when reading code where it is in the right place, then you immediately see why it could get keys that do not fit for that particular template without ding harm and it fits with the underlying command which is \keys_set_known:nnN. But I'm not insisting on a change, just think that it might be result in a slightly better code reading.

Not that I have much to say about this, but if this should only be used inside template code how about something like \SetThisTemplatesKeys or \SetTemplateInstanceKeys? (Please ignore if this sounds stupid)

@FrankMittelbach
Copy link
Member Author

I like the documentation changes. I'm not sure I fully understand the discussion about known/unknown keys, but I'm not a fan of the lack of error when an unknown key is passed to, say, \begin{enumerate}. It's too easy to misremember leftmargin as left-margin or left margin and be confused why nothing is changing.

The problem is that as @FrankMittelbach has indicated, you might need several templates for a complex case like this. So how do you decide where to issue a warning? We really don't want users to have to work out which template is which for setting keys - we looked at prefixing all keys, and it doesn't make a good interface.

I have now a first implementation for checkingg all user-supplied keys to block environments.

a) it can be handled (but not yet cleanly, I'm missing some support interfaces)
b) it showed me that you probably don't want \SetTemplateKeys but only \SetKnownTemplateKeys because the error handling of the former is inferior and exposes the internal template structure and one can do much better to do it manually at the end..
c) current implementation is still WIP

@car222222
Copy link
Contributor

@FrankMittelbach @josephwright
It seems that this is still ongoing, so I shall not review it until it is finalised.

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.

7 participants