brucemiller / LaTeXML

LaTeXML: a TeX and LaTeX to XML/HTML/ePub/MathML translator.
http://dlmf.nist.gov/LaTeXML/
Other
961 stars 101 forks source link

Rename xsl context param to more targeted spansoup #2217

Closed dginev closed 1 year ago

dginev commented 1 year ago

This PR only contains a cleanup renaming (hopefully).

Motivated by the context and mode keywords in our XSLT files being too similarly vague to encounter in the same template, and trying to pin down more accurate names.

In the process I also moved innercontext to inline, since it was always set to the constant value of 'inline'. I am even wondering if that needed a dedicated variable or a simple string would do?

Certainly more tweaking is possible here (e.g. now that we are claiming the param is a simple boolean toggle, it can be changed to using a boolean true/false). But it's a start resolve some of the naming difficulties I had.

brucemiller commented 1 year ago

OK, sounds like fun :>

xworld21 commented 1 year ago

Ouch! For us using custom stylesheets, this is quite the breaking change. Lots of templates receive and pass around context and will need to be rewritten. BookML alone has some ~140 mentions of the context parameter, and I have to support multiple versions of LaTeXML, which will make this update even more difficult to handle (you can't interpolate a parameter name as far as I know).

(Incidentally, I find spansoup less clear, it says what happens, but not why. Also strange that it is spansoup = 'inline' rather than say spansoup = true().)

brucemiller commented 1 year ago

I'd be OK retracting this, but I'll let you two debate it.

dginev commented 1 year ago

I admit this is unfortunate.

It is also a bit clunky that we don't have a versioning scheme which clearly marks a breaking change has been enacted, so that everyone else downstream can prepare - and guard against it. Indeed if this remains merged I would expect BookML to need its own dedicated version that has latexml 0.8.8 as a minimal requirement.

context is a bad name for a boolean toggle IMHO, especially when mode is also used, and sometimes used to mean a named context (mode="description" was the example I gave Bruce).

Changing the underlying value to a boolean true()/false() is something I considered - and still sounds reasonable - but I didn't want to change behavior with my first suggestion. I'm open to also switching to that, and simplifying some further details (the $inline variables don't seem needed).

Last but not least, the "context" parameter looks odd when I was considering more sophisticated templates with multiple independent pieces of contextual information. XSLT1 has no way of adding further dimensions of data to a single parameter (with the exception of node-set() black magic I think). And having a "context" param adjacent to other "contextual parameters" was extra confusing to me.

brucemiller commented 1 year ago

I haven't yet seen a "versioning scheme" that would make this kind of change not be a pain (I hadn't realized how much customized XSLT was out there). And I'm not sure how XSLT1 (or any language) accommodates (or should) a single function/template argument suddenly meaning acquiring whole new sets of additional dimensions. But...

|context| is a bad name for a boolean toggle IMHO,

It would be, but context is not a boolean toggle; it states the current (html output) context. As it turns out the only practical effect so far depends on whether it is "inline" or not. Maybe, in hindsight, a longer name would have been clearer, but it is always a balancing act between clarity, typeability and readability.

dginev commented 1 year ago

@brucemiller if you've decided to revert - you're obviously welcome to. We don't actually need to debate this kind of change. We already had a conversation about it last Thursday, I think that was as much time as we can reasonably spend on a naming question.

Let me know when you decide either way, since as Vincenzo says - future XSLT work needs to use the right parameter name, whichever string that happens to be.

brucemiller commented 1 year ago

Well, I hadn't quite decided, but I guess I have now.

Now, I've got to figure the safest way to revert the change, given that I clumsily merged another PR on top of it.

brucemiller commented 1 year ago

Pushed commit 41dcd5eedd3246a5b53ac8a5bb9ad909d9ac135b to revert this.

xworld21 commented 1 year ago

Thanks! Of course I don't want to hold you back, and I have coped with other breaking changes in the past (e.g. the spinners in 0.8.5 or .6), it's just that XSLT is really inflexible sometimes, and in this case the change is just a no-op, functionally speaking, so it felt disproportionate.

Re spansoup, booleans, I'd figure that something like content-category is more descriptive as it refers specifically to HTML terminology. I believe "inline context" is "phrasing content" in HTML, the other category being "flow content". I could imagine you extending this with e.g. an extra value "flex", which behaves like flow content but can be detected to offer a flex-friendly output.

Anyway, I have had some ideas for how to handle this kind of change on the BookML side (some replacing <xsl:apply-templates> with a named template that switches parameters according to the version – clunky, but if it works I would stop complaining on your XSLT PRs). I'll let you know if how that goes.