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

Ordering rules for cleveref and various other packages #2

Merged
merged 7 commits into from
Aug 29, 2014
Merged

Ordering rules for cleveref and various other packages #2

merged 7 commits into from
Aug 29, 2014

Conversation

liblit
Copy link
Contributor

@liblit liblit commented Aug 29, 2014

No description provided.

Most of these come from `\@ifpackageloaded` checks in `cleveref.sty` that
add special support if the other package is already loaded.  One is also
mentioned explicitly in the `cleveref` documentation.
The `cleveref` documentation explicitly states the order in which these
must be loaded if all three are used.  Other rules already put `hyperref`
and `varioref`  before `cleveref`.  This adds the final necessary
constraint to enforce the correct total ordering among all three packages.
\Load {listings} before {cleveref} if loaded because {cleveref adds special support for this package if it is already loaded}
\Load {ntheorem} before {cleveref} if loaded because {cleveref adds special support for this package if it is already loaded}
\Load {subfig} before {cleveref} if loaded because {cleveref adds special support for this package if it is already loaded}
\Load {varioref} before {cleveref} if loaded because {cleveref adds special support for this package if it is already loaded}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. Thanks for this contribution! Three comments:

  1. I can't find the loading order preference of amsmath and caption in the cleveref docs. It mentions them in other places, but (how) are you certain they have to be loaded first?

  2. Most cleveref rules are very similar and can be abbreviated as follows:

    \Load {cleveref} after {algorithm2e,algorithmicx,...} if loaded because {...}

  3. At this point it would make sense to group these rules by source. Could you please separate your new rules from the pre-existing ones and specify their source with a simple comment block (with a URL)? I should have done the same for the existing rules a while ago. (Maybe, while you're at it, could you specify that those came from "http://www.macfreek.nl/memory/LaTeX_package_conflicts"?)

Thanks!

@liblit
Copy link
Contributor Author

liblit commented Aug 29, 2014

The preference to load amsmath and caption before cleveref comes from inspecting cleveref.sty by hand, specifically looking for uses of \@ifpackageloaded. From this it is clear that cleveref has special support for both amsmath and caption, activated only if those other packages are already loaded. I claim that the implementation is more truthful than the documentation here.

I like your suggested reorganization of the many similar rules. I will make that change, which should then propagate into this existing pull request. I'll add the clarifying attribution for the existing rules too, per your request.

@liblit
Copy link
Contributor Author

liblit commented Aug 29, 2014

Actually, one clarification regarding caption: the special cleveref handling is only triggered if caption is from before August 19, 2011. Presumably caption released after that date no longer needs any special handling. So do you think we should keep this ordering constraint or drop it?

@mhelvens
Copy link
Owner

The preference to load amsmath and caption before cleveref comes from inspecting cleveref.sty by hand, specifically looking for uses of \@ifpackageloaded.

Is it possible that cleveref is cleaning up after itself here, i.e., that the special handling is not needed if amsmath/caption are loaded later? Not having looked at the relevant cleveref code, I can't be sure. But I can imagine a scenario where two packages each have special handling for each other. Adding both rules would then lead to a dependency cycle.

That said, if you are reasonably confident about these, please keep them in.

Actually, one clarification regarding caption: the special cleveref handling is only triggered if caption is from before August 19, 2011. Presumably caption released after that date no longer needs any special handling. So do you think we should keep this ordering constraint or drop it?

I'm thinking about a feature where a rule can be conditional on package date/version/options. But that's not implemented yet.

mhelvens added a commit that referenced this pull request Aug 29, 2014
Ordering rules for `cleveref` and various other packages
@mhelvens mhelvens merged commit 2f61d24 into mhelvens:master Aug 29, 2014
@mhelvens
Copy link
Owner

Thanks again! I've merged your changes, and I'll release a new version to CTAN soon(ish).

@liblit
Copy link
Contributor Author

liblit commented Aug 29, 2014

I definitely see what you're saying about whether cleveref is cleaning up after itself or really does need to be loaded later. It's hard to tell; my TeX-reading skills are not up to the task. I can say that when cleveref does its special work for some other package, it almost always prints in informational message '_otherpackage_' support loaded. The one exception is amsmath, for which cleveref triggers special handling silently. I don't know how much we should read into that, though; it might just be that the support loaded message was accidentally forgotten in the amsmath-handling code.

I'll definitely want to send you another pull request (since you already merged this one) that removes the caption ordering rule, per your comment above. Should I also remove the amsmath rule?

@mhelvens
Copy link
Owner

In fact, it seems to me that both rules are probably fine (for now), since the cleveref author is obviously aware of those other packages. So loading them first is at the very least 'not wrong'.

The recommended ruleset doesn't have a cycle in it yet, so let's keep it the way it is for the moment. In the future, we'll want to come up with some proper guidelines about which rules should be included.

@liblit
Copy link
Contributor Author

liblit commented Aug 29, 2014

OK, sounds good to me. We're done here!

@liblit liblit deleted the cleveref branch August 29, 2014 21:33
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.

None yet

2 participants