Definition of \storeordinal

[eg9]

The current definition of \storeordinal is

\newcommand*{\storeordinal}[2]{%
  \expandafter\protect\expandafter\storeordinalnum{#1}{%
    \expandafter\the\csname c@#2\endcsname}%
}

Looking at the action of the first \expandafter it is clear it does nothing, because it expands the second one, which in turn tries to expand the opening brace.

Also the third \expandafter is redundant, because \the does expansion in order to find a legal token after it. So, assuming \protect is necessary (which I'm doubtful of), the definition should be

\newcommand*{\storeordinal}[2]{%
  \protect\storeordinalnum{#1}{\the\value{#2}}%
}

as \value{#2} expands to \csname c@#2\endcsname.

[vincentb1]

Concerning the first \expandafter, I guess that there is a missing \expandafter just before the opening curly brace, and the intention was to expand once #1 before passing it to \storeordinalnum.

Concerning the second \expandafter, I agree with you.

So, all in all, it could corrected to:

\newcommand*{\storeordinal}[2]{%
  \expandafter\protect\expandafter\storeordinalnum\expandafter{#1}{%
    \the\value{#2}}%
}

I don't think that we should remove the \protect, even though I don't have in mind any use case where it is needed. All the more that \storeordinalnum is defined with a plain \newcommand* and not a \DeclareRobustCommand.

[nlct]

I don't know why I originally put those \expandafters in. I don't think they're needed as the first argument is just a label. I think we can probably remove the \protect as there's some protection within the internal commands used by \storeordinalnum but if you want to err on the side of caution we can leave it in.

[vincentb1]

Ok, in an nutshell, would that be :+1: ? :

\newcommand*{\storeordinal}[2]{%
  \protect\storeordinalnum{#1}{%
    \the\value{#2}}%
}

    Vincent

[vincentb1]

Ok, finally as nobody was barking, I did the changes with tagging them as v3.02 and pushed everything to repo.

Please note that I also suppressed the \protect, and instead used DeclareRobustCommand in place of \newcommand for defining \storeordinalnum. I also did similar changes for \ordinalnum and \FCordinal.

Hopefully that makes everybody happy.

Nicola @nlct, do you agree to close this bug ?

[nlct]

(Sorry, I've been off-line a lot lately.) I realise where the error probably came from. I must've copied the code from \FCordinal and then edited it into \storeordinal. The code for \FCordinal:

\newcommand{\FCordinal}[1]{%
  \expandafter\protect\expandafter\ordinalnum{%
    \expandafter\the\csname c@#1\endcsname}%
}

The \protect is designed to ensure that if \FCordinal{counter} is written to the .aux file (for example, through \label) then it gets expanded to \ordinalnum{number}. This is necessary to ensure that the counter name is replaced by the number (otherwise the counter will have a different value when the .aux file is processed). This, of course, should have had an extra \expandafter before { (or remove the \expandafters) but the \protect must remain within \FCordinal. So \FCordinal can just be

\newcommand{\FCordinal}[1]{%
  \protect\ordinalnum{\expandafter\the\csname c@#1\endcsname}%
}

Since \storeordinal isn't likely to be used in this context, I think it can simply be

\newcommand*{\storeordinal}[2]{\storeordinalnum{#1}{\value{#2}}}

(and possibly made robust). In this case \the isn't needed as the value of the counter isn't being written to a file.

[vincentb1]

Hello, Just to tell that I have included into the test bench tests that verify that \FCordinal and \numberstring are ok to be used within sectionning commands (like \section) and with a table of content. This seems to be a good test for robustness, isn't it ?

For \FCordinal, the test works because \ordinalnum is now declared with \DeclareRobustCommand*, like this:

\DeclareRobustCommand*{\ordinalnum}[1]{%
  \new@ifnextchar[%
  {\@ordinalnum{#1}}%
  {\@ordinalnum{#1}[m]}%
}

Please note that \FCordinal is in fact just now simplified:

\newcommand{\FCordinal}[1]{%
  \ordinalnum{%
    \the\value{#1}}%
}

I think it would be better to re-introduce the \expandafter — I had removed them because as one was missing, they were anyway not doing anything — so I would like to make it as follows — so we don't rely on \ordinalnum making any expansion:

\newcommand{\FCordinal}[1]{%
  \expandafter\ordinalnum\expandafter{%
    \the\value{#1}}%
}

For \numberstring, this works because it is declared in a way that inherently makes it robust (contains \protect within the definition), like this:

\newcommand*{\numberstring}[1]{%
  \expandafter\protect\expandafter\numberstringnum{%
    \expandafter\the\csname c@#1\endcsname}%
}

Well, this is no so consistent, it would be better if \numberstringnum was declared with \DeclareRobustCommand*, and \numberstring would be simplified — no more \protect but certainly it would be good to keep the \expandafter for the same reason as I want to re-introduce them in \FCordinal. So \numberstring would become:

\newcommand*{\numberstring}[1]{%
  \expandafter\numberstringnum\expandafter{%
    \the\value{#1}}%
}

and in definition of \numberstringnum, I would replace \newcommand* by \DeclareRobustCommand*.

Also, all the other command \xxxxnum (like \Ordinalstringnum) should be declared with \DeclareRobustCommand* instead of \newcommand* for consistency.

If you (@nlct) don't object, I will do that…

[vincentb1]

Another possibility, would be to keep the \newcommand* for the \xxxxnum commands, but make it followed by a \robustify\xxxxnum, so using the \robustify of etoolbox.

Any opinion on that ?

[nlct]

(Sorry for the delay in replying I've been away for a couple of weeks.) The tests need to include cross-referencing. For example:

\documentclass{article}
\usepackage{fmtcount}

\renewcommand{\thesection}{\numberstring{section}}% or \ordinalstring etc

\begin{document}
\section{Sample}
Test cross-reference to section~\ref{sec:test}.
\section{Test}
\label{sec:test}
\end{document}

The counter must get expanded to its current value in the aux file. So in the above, we need \numberstringnum {2} in the aux file. I agree that the \xxxnum commands can all be robust. That will prevent any further expansion. If we have etoolbox then we could simply use \newrobustcmd. I'm happy for you to make those changes if you like.

[vincentb1]

Hello, I must say that there was already some cross-reference testing in the fc-sample. Please have a look at trunk/test/reference/fc-samp.txt lines 42-43, and you can read:

2nd Cross-Referencing
Referencing a label: 1st.

Of course this cross-reference testing was not so extensive because only \ordinal and english were tested. So I have added more tests with \numberstring \Numberstring \NUMBERstring \ordinalstring \Ordinalstring \ORDINALstring \ordinal. The good news is that without any changes in fmtcount.sty everything is OK in the output.

BTW, I have moved sample files for tests to the test subdirectory, as I had suggested to do quite some time ago.

[vincentb1]

Hello, I have done all the planed job, and added additional tests for that. FYI, I could not find that any of these changes were really useful. All the tests which I have run with the new code, also are ‘passed’ with the previous code. At least, code is hopefully now more systematic and easier to maintain.

Anyway, it was good to make this changes, because that will simplify solving issue #11 — now I will just replace the \DeclareRobustCommand by \newcommand* in the fc-xxxx.def files, and that's it for dynamical loading.