bc-tools / for-latex

Tools specific to LaTeX coding and co.
2 stars 1 forks source link

Code recommendations #2

Open cfr42 opened 3 weeks ago

cfr42 commented 3 weeks ago

I looked at your .cls file on CTAN and noticed a couple of things you might want to fix.

The first is that you are loading the same packages several times, sometimes with different options and/or versions. This is bad practice and a recipe for bugs. I also wondered whether your code really did require the versions you specify. Generally, people do this when their code requires a new feature or fix. By specifying particular versions, especially very recent ones, you make it hard for people to e.g. add your package to a project on Overleaf or some other managed environment, since they may well find they need a whole bunch of other things, too.

The second is that you're loading hyperref early. Even if that's not a problem for the packages the class loads later, it may well cause problems if a user loads packages in the preamble. For example, somebody might want to load biblatex or might want to use bookmark. One way around this would be to use a hook at the start of the document which tests whether hyperref has been loaded (e.g. by bookmark) and does the configuration if not. (You don't strictly need to test if it is loaded as LaTeX won't load it twice, but you might want to avoid \hypersetup in that case.)

projetmbc commented 3 weeks ago

Thanks for your comment.

Some packages loaded several times

I plead guilty. I use a specific workflow and finalize the whole project via a homemade Python module which is still imperfect. The next step is to turn this Python module POC into a real module available on PyPi.

The versioning

I use semantic versioning: https://semver.org . Minor updates indicate new features in the project itself.

Loading hyperref

I will integrate this into the next minor version via \AtBeginDocument. The use of \hypersetup is limited to the colors of the links to obtain a coherent color system.

cfr42 commented 2 weeks ago

I think you misunderstood my comment about versioning. I'm not criticising your versioning methodology. (I know CTAN recommends so-called semantic versioning1.)

The versioning

I use semantic versioning: https://semver.org . Minor updates indicate new features in the project itself.

I was talking about the fact that your package specifies the versions of other packages it loads i.e. it will error if the available packages are older than the versions you specify. That's perfectly reasonable if you really depend on something only available in more recent versions, but I couldn't help getting the impression that you specify versions routinely and more-or-less arbitrarily when I see, for example (refs: https://github.com/bc-tools/for-latex/blob/b2da577fdf1b0e534b463f2ca5225207a3755ad8/tutodoc/src/main/main.cls#L38 and https://github.com/bc-tools/for-latex/blob/b2da577fdf1b0e534b463f2ca5225207a3755ad8/tutodoc/src/main/main.cls#L59):

\RequirePackage[svgnames]%
               {xcolor}%
               [2023/11/15]
...
\RequirePackage{xcolor}%
               [2022/06/12]

Aside from that you are (1) loading the same package twice and (2) loading it with different options, you are also specifying different versions. Now, imho, if you require a package, you should have good reason for it and, if you require a particular version of that package, you should have good reason for that. Specifying version-such-and-such-or-later should be the exception, I would think, rather than the rule.

Perhaps I am mistaken, but I have the impression you simply add the current version when adding any class or package at all, regardless of whether it is needed.

1I don't mean it isn't semantic, but I object to the use of 'semantic' to differentiate it from all other versioning schemes. What they mean is something like 'standard'. Any sane versioning scheme is semantic.

projetmbc commented 1 week ago

Effectively, I was wrong. Once again, the repeated packages and inconsistent version numbers will be corrected in the next version. However, the version number of an imported package is a minimum number: https://tex.stackexchange.com/a/47766/6880 . I prefer to indicate this concerning the tests I carried out locally.

PS: regarding Overleaf, if there is a GitHub action for testing related to this project, I will integrate it into my future workflow.

projetmbc commented 2 days ago

For the next version, which I'm working on, there will be no more duplicate packages, versions/dates will no longer be indicated unless there is a compelling need, and the documentation will indicate the versions/dates of the packages when the tests have been done.