ho-tex / doi

The small doi package for linking to doi.org
8 stars 2 forks source link

Optional loading of hyperref #5

Open janniklasrose opened 3 years ago

janniklasrose commented 3 years ago

This package relies on hyperref for the href command. However, loading it directly with \RequirePackage{hyperref} can cause problems for authors who place \usepackage{doi} early in their preamble and thus violate the rule that hyperref should be loaded last. One workaround is to load doi after hyperref, but this is not documented in the doi package.

This PR adds the package options hyperref and nohyperref. The former matches the default (legacy) behaviour to call \RequirePackage{hyperref} while the latter omits this and leaves it up to the user to include the hyperref package separately.

janniklasrose commented 3 years ago

I also tried to implement something with \@ifundefined{\href}{}{\href} but could not get it to work.

Before merging, I would also like to add some documentation changes to this PR to tell users in the README about the options and what they mean regarding hyperref.

I'd love to hear your thoughts on this

davidcarlisle commented 3 years ago

I'm not sure about this to be honest adding the package option mechanism seems a relatively heavyweight solution. Documenting doi should be late is not really any different than documenting hyperref should be loaded late.

This is already documented in the package comments:

%% We need hyperref, but you probably want to load hyperref 
%% beforehand,

I am not sure how the package option really helps. If the document author has to edit to add a nohyperref option but still load hyperref later, it would be simpler for them to not add the option and move the doi package after hyperref.

If you wanted to change the code here it would I suspect be possible for the package to use \AtBeginDocument to delay loading hyperref.

janniklasrose commented 3 years ago

My rationale for this is that I use biblatex, which should be loaded before hyperref, together with doi, which should be loaded after hyperref. But for my own sanity and general readability, I would prefer to keep biblatex and doi close to one another in the preamble. It is also confusing if the doi package is loaded in another .sty file, which obscures the dependency on hyperref.

I agree that a package option is probably overkill and not ideal either, but I'm not sure if using \AtEndPreamble{\RequirePackage{hyperref}} is a good/better solution. Ideally, something like @ifundefined{\href}{...}{...} would avoid the use of \RequirePackage{hyperref} altogether and result in expected behaviour - loading doi.sty only provides \doi and additionally loading hyperref makes those clickable. If you agree with this, I can try to investigate how to get this working?

P.S. Regarding documentation of this:

This is already documented in the package comments

From the README.md of this repo, the only documentation available on CTAN, it is not immediately obvious that this package loads hyperref (a complex package). Perhaps a note could be put there, because not every author reads or understands the .sty source code. -- this becomes irrelevant in case of the \@ifundefined fix

davidcarlisle commented 3 years ago

not loading hyperref and doing a run-time check would have been OK as an original design but if we do that now, potentially existing documents using doi package but not explicitly loading hyperref would lose hyperref features and break. We can certainly do something but I'm not sure what (definitely we can improve the readme) Perhaps @u-fischer might have a more informed view as she's closer to hyperref internals currently.

I probably can't think about this for a couple of days in any case so I'll leave this open. Feel free to add any additional thoughts you have (but I may not respond to each post)

janniklasrose commented 3 years ago

Thanks for the feedback, curious to hear from @u-fischer.

I just played around with my local copy of doi.sty and implemented some \ifdefined logic that seems to work - but as you said would be a breaking change. I have nonetheless committed my hacky solution in b9116b0 to preserve it for posterity.

u-fischer commented 3 years ago

I don't understand why you need this package if you use biblatex. Biblatex has its own commands to link a doi, e.g. in biblatex.def you can find

\DeclareFieldFormat{doi}{%
  \mkbibacro{DOI}\addcolon\space
  \ifhyperref
    {\href{https://doi.org/#1}{\nolinkurl{#1}}}
    {\nolinkurl{#1}}}