Note: This review is not based on the code in above's answer, but on commit 28c7fdc.
----
In this review I'll first address your code directly and give some nagging on the code as it is, not on primarily on its general behaviour. After that I'll present my thoughts on the behaviour.
----
# Nitpicky code review on file `tipauni.dtx`
## Initialisation of the variables (line 523ff):
```
\newif\ifcharissil
\newif\ifpreservefont
\newif\ifdocumentfont
\newif\iffontoptions
\newif\ifnontipaignore
```
Those are internal macros and should be named accordingly, e.g., `\newif\iftipauni@charissil` etc.
```
\newcounter{tipauni@settings}
\setcounter{tipauni@settings}{3}
```
Do you really need a counter for this? You could as well define a macro to save the state (still with numeric values), or use a count register directly (LaTeX's `counter` mechanism is a bit overkill for storing an internal state, as it includes printing the counter, and potential reset hooks for other counters, etc.; though the latter is not invoked by `\setcounter`, only by `\(ref)stepcounter`).
Suggested code:
```
\newcount\tipauni@settingsstate
\tipauni@settingsstate=3
```
(the missing `%` after 3 to suppress the line ending from adding a space is deliberate, this way the space terminates the numeric expression and you don't accidentally expand things in the next line that shouldn't be expanded just yet, if you prefer you could also use `\tipauni@settings=3\relax` to make this more obvious)
## Definition of the `preservefont` option (line 554ff):
```
\DeclareOptionX{preservefont}{%
\documentfontfalse
\charissilfalse
\preservefonttrue
\ifnum\value{tipauni@settings}<3%
```
Is there any possibility of `tipauni@settings` getting bigger than 3? If not, just omit the test and use `\setcounter{tipauni@settings}{3}`, gives clearer code, KISS.
```
\setcounter{tipauni@settings}{3}%
\else
```
Empty `\else` branch, you could drop the `\else`.
```
\fi
}
```
Suggested alternative implementation:
```
\DeclareOptionX{preservefont}{%
\documentfontfalse
\charissilfalse
\preservefonttrue
\setcounter{tipauni@settings}{3}%
}
```
## Definition of the `resetfontspecoptions` option (line 578ff):
```
\DeclareOptionX{resetfontspecoptions}{%
\ifx\tipauni@font@options\undefined
```
That condition can never be met, line 546 did `\let\tipauni@font@option\@empty`, and no option undefines it, so this `\ifx` test can be omitted.
```
\else
\let\tipauni@font@options\@empty
\fi
\fontoptionsfalse
}
```
Suggested alternative implementation:
```
\DeclareOptionX{resetfontspecoptions}{%
\let\tipauni@font@options\@empty
\fontoptionsfalse
}
```
## Definition of the `documentfont` option (line 590ff):
```
\DeclareOptionX{documentfont}{%
\edef\tipauni@temp{#1}%
\ifx\tipauni@font\undefined
```
You could use `\ifdefined` instead (*Attention* this would invert the logic, so true and false branch need to be swapped).
```
\ifx\tipauni@temp\@empty
\ifpreservefont
\setcounter{tipauni@settings}{0}%
\documentfontfalse
\charissilfalse
\else
\setcounter{tipauni@settings}{1}%
\documentfontfalse
\charissilfalse
\fi
```
No code doubling! If something happens in both cases don't put it inside both if-branches, instead just move it outside of it.
```
\else
\ifnum\value{tipauni@settings}<3%
\setcounter{tipauni@settings}{3}%
\else
\fi
\edef\tipauni@font{#1}%
```
Unnecessary `\edef`, just `\let\tipauni@font\tipauni@temp`.
```
\documentfonttrue
\charissilfalse
\fi
\else
\ifx\tipauni@temp\@empty
\documentfonttrue
\charissilfalse
\setcounter{tipauni@settings}{2}%
```
Isn't this running into an open knife? You know that no font has been provided yet (`\tipauni@font` is still undefined), and the given option doesn't hold a valid font (`\tipauni@temp` being empty). This should throw an error, not only a warning (the warning is thrown further down the road, in line 713ff; and this is the only case setting `tipauni@settings` to 2)!
```
\else
\documentfonttrue
\charissilfalse
\edef\tipauni@font{#1}
```
Why doesn't this `\setcounter{tipauni@settings}{3}`? This should be a valid case, shouldn't it?
```
\fi
```
Again code doubling and `\edef`! (and `\charissilfalse` is used in every case!)
```
\fi
}
```
Suggested alternative implementation (this contains a behaviour change, namely it would set `tipauni@settings` to 3 if the given argument wasn't empty regardless of the previous state of ``\tipauni@font`):
```
\DeclareOptionX{documentfont}{%
\charissilfalse
\documentfonttrue
\edef\tipauni@temp{#1}%
\ifx\tipauni@temp\@empty
\ifdefined\tipauni@font
\setcounter{tipauni@settings}{2}%
\else
\documentfontfalse
\ifpreservefont
\setcounter{tipauni@settings}{0}%
\else
\setcounter{tipauni@settings}{1}%
\fi
\fi
\else
\let\tipauni@font\tipauni@temp
\setcounter{tipauni@settings}{3}
\fi
```
## Definition of the `recommendedfont` option (line 667ff)
```
\DeclareOptionX{recommendedfont}{%
\documentfonttrue
\def\tipauni@font{CharisSIL}%
\setcounter{tipauni@settings}{3}%
}
```
Why not just `\charissiltrue` and `\documentfonttrue`?
## Error parsing (line 684ff):
```
\ifnum\value{tipauni@settings}=0%
```
Do you know the `\ifcase` primitive?
```
\PackageWarningNoLine{tipauni}{%
1. The `preservefont' option is active.\MessageBreak
2. The `documentfont' option has no value.\MessageBreak
Your document will be compiled with the default\MessageBreak
font-family of (Xe/Lua)LaTeX, i.e., Latin Modern\MessageBreak
unless another font is set with tipauni-external\MessageBreak
methods%
}%
```
If the `preservefont` option is active this should be a no-issue, as it should mean "don't touch the font setup" which doesn't conflict with `documentfont` having no value, the real error is that someone used `documentfont` with an empty value, which should always be an error, shouldn't it (at least it is not documented that there is a special "empty value" syntax)?
```
\else
\ifnum\value{tipauni@settings}=1%
\PackageWarningNoLine{tipauni}{%
The `documentfont' option has no value. Your\MessageBreak
document will be compiled with the default\MessageBreak
font-family of (Xe/Lua)LaTeX, i.e., Latin Modern\MessageBreak
unless another font is set with tipauni-external\MessageBreak
methods%
}%
\fi
\ifnum\value{tipauni@settings}=2%
\PackageWarningNoLine{tipauni}{%
A `documentfont' option has no value, but you have\MessageBreak
loaded another font with some tipauni-option, so that\MessageBreak
font will be used as the main font of this document\MessageBreak
unless another font is set with tipauni-external\MessageBreak
methods. The font being currently loaded is:\MessageBreak
\tipauni@font%
```
Can this branch be actually called with a non-empty `\tipauni@font`?
```
}%
\else
\fi
\ifnum\value{tipauni@settings}=3%
\else
\fi
```
Neither the true nor the false branch are used, so why bothering checking this case anyway?
```
\fi
```
Suggested code:
```
\ifcase\value{tipauni@settings}% case 0
\PackageWarningNoLine{tipauni}{%
1. The `preservefont' option is active.\MessageBreak
2. The `documentfont' option has no value.\MessageBreak
Your document will be compiled with the default\MessageBreak
font-family of (Xe/Lua)LaTeX, i.e., Latin Modern\MessageBreak
unless another font is set with tipauni-external\MessageBreak
methods%
}%
\or% case 1
\PackageWarningNoLine{tipauni}{%
The `documentfont' option has no value. Your\MessageBreak
document will be compiled with the default\MessageBreak
font-family of (Xe/Lua)LaTeX, i.e., Latin Modern\MessageBreak
unless another font is set with tipauni-external\MessageBreak
methods%
}%
\or% case 2
\PackageWarningNoLine{tipauni}{%
A `documentfont' option has no value, but you have\MessageBreak
loaded another font with some tipauni-option, so that\MessageBreak
font will be used as the main font of this document\MessageBreak
unless another font is set with tipauni-external\MessageBreak
methods. The font being currently loaded is:\MessageBreak
\tipauni@font%
}%
\fi
```
Also change the "everything is fine" case to `-1` instead of `3`. Rationale: If you get additional error cases you can just add them to the above `\ifcase...\or...\fi` structure, `-1` is never reached by it (only half true, `\ifcase` supports an `\else` that handles every unhandled case).
-----
# General behaviour rambling
Why do you bother evaluating the settings on the fly to set `tipauni@settings`? In the end you throw any warnings/errors only after the fact, so why not evaluate the options only by then? The current implementation makes your options much more complicated than they have to. Compare all of above with the following (admittedly not the exact same behaviour, but should cover the bare necessities and give you an idea on what I mean -- sorry for ):
```
\newcommand*\tipauni@font@options{}
\newif\iftipauni@preserve
\newif\ifnontipaignore
\DeclareOptionX{preservefont}{\tipauni@preservetrue}
\DeclareOptionX{fontspecoptions}{\edef\tipauni@font@options{#1}}
% I'd expect this option to do more than just `options={}`
\DeclareOptionX{resetfontspecoptions}
{%
% in this implementation the undefined variable caries the information of
% your `\charissiltrue`
\let\tipauni@font\tipauni@undefined
\let\tipauni@font@options\@empty
}
\DeclareOptionX{documentfont}{\edef\tipauni@font{#1}}
\DeclareOptionX{recommendedfont}{\def\tipauni@font{CharisSIL}}
\DeclareOptionX{incompatible}{\nontipaignoretrue}
\ProcessOptionsX\relax
\iftipauni@preserve
\ifx\tipauni@font@options\@empty
\else
\PackageWarning{tipauni}
{%
You specfied font options while `preservefont` is active.\MessageBreak
Either completely set your font options outside of tipauni\MessageBreak
or leave out the `preservefont` option.%
}%
\fi
\else
\ifx\tipauni@font\@empty
\PackageError{tipauni}{Empty documentfont option}
{%
If you want tipauni to not set any font related options use the
preservefont option%
}%
\else
\ifdefined\tipauni@font
\else
\def\tipauni@font{CharisSIL}
\fi
\expandafter\RequirePackage\expandafter[\expandafter{\tipauni@font@options}]
{\tipauni@font}%
\fi
\fi
```
You don't win anything from evaluating everything on the spot if you don't act on it. Either check whether `documentfont`'s value is empty and if so throw an error immediately, or don't only check this when you actually need to access this. Since you only need to access this once, but the possibility exists that it is set multiple times, I'd opt for lazy evaluation instead of an eager approach.