-
-
Notifications
You must be signed in to change notification settings - Fork 251
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
Extend \Make...case to support languages #936
Conversation
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.
in general looks good to me, a couple of points raised by
\documentclass{article}
\begin{document}
\MakeUppercase{xyz}
\MakeUppercase[]{xyz}
\MakeUppercase[langg=en]{xyz}
\MakeUppercase[lang=fr]{xyz}
\typeout{\MakeUppercase{xyz}}
\typeout{\MakeUppercase[lang=fr]{xyz}}
\end{document}
I wonder if it is still necessary to use a \protected
inner command as the whole code is expandable. The typeouts produce
\MakeUppercase []{xyz}
\MakeUppercase [lang=fr]{xyz}
rather than XYZ which is more or less compatible behaviour. Previously it was necessary but I'm not sure it would often have been required, and we could probably change this to allow expansion?
Other point shows up in the error message, the keys are in a generic kernel group but should (probably?) be in a specific group for this so they don't get mixed in with other keys as we add more kv interfaces in the format.
If we want keys, then working by expansion is a bit more tricky (though doable). If we decide we only want a simple optional argument, that would be easier. |
I was imagining that |
If we want keys, then working by expansion is a bit more tricky (though doable). If we decide we only want a simple optional argument, that would be easier.
Yay, `expkv-cs` :P
More seriously: Do you expect any more keys than the `lang` key? I don't
see how the case-changers would need so many more keys. So I'd go this
route:
```
\NewExpandableDocumentCommand \MakeUppercase { O{\l__kernel_lang_key_tl} m }
{ <code> }
```
|
Well there's a |
The feeling was e.g. tagging |
Yes, I think one should keep the option open for keys to add a structure or an alternate text. |
Thoughts on the key names would be welcome: I think |
Indeed, even if we do keys by expansion, any tagging is non-expandable. |
I use |
Fully agree with @u-fischer here, KISS for key names. |
[Very busy these last few days.] It’s ok for me, even This option can be useful in some cases, but from the point of view of localization, the macro, without any changes or options, must upper- or lowercase a string according to the current rules, even if tailored by the user. So, |
I'd noticed :)
I'd seen that HTML uses
Indeed: that is I hope covered by 639e761, which is part of the PR. That change should mean 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.
looks fine to me with some minor comments
base/ltfinal.dtx
Outdated
lang .meta:n = { locale = {#1} } , | ||
locale .str_set:N = \reserved@a |
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 wonder what will be the predominant use, by guess is "lang" not "locale" so perhaps the .meta should be the other way around (not really important though)
base/ltfinal.dtx
Outdated
% The odd use of \emph{three} spaces here is needed as \pkg{ltcmd} uses the | ||
% name with one and two spaces to give a `friendly' error message for a runaway | ||
% argument: that means we can't use it here. |
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'm probably dense (or the diff is not showing what I need) but which 3 spaces are we talking about?
\cs_generate_variant:cn { text_ \str_lowercase:n {#1} case:nn } { V } | ||
\cs_new_protected:cpx { Make#1case \c_space_tl \c_space_tl \c_space_tl } [##1] ##2 |
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.
ahh these, so perhaps the comment should really be here
95cf458
to
6aec1a4
Compare
This PR focussed on language support in two ways: