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

Optional loading of hyperref #5

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

janniklasrose
Copy link

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.

Add package options hyperref (default) and nohyperref
@janniklasrose
Copy link
Author

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
Copy link
Member

davidcarlisle commented Aug 12, 2021

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
Copy link
Author

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
Copy link
Member

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)

Makes hyperlink creation optional and dependent on the user having loaded hyperref
@janniklasrose
Copy link
Author

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
Copy link

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}}}

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.

3 participants