Pub attributes - Githubissues

Alex-Jordan commented 1 year ago

I was going to apply the pubfile infrastructure to some things like you suggested a while back. But then I saw how "free form" variables like the webwork server could also fit into the new infrastructure. So I did that instead for now and applied it to the webwork server and related variables.

I did it by using the wildcard * as an option for an attribute if it is allowed to be free form. There are alternative ways to do this if you think a * might one day be a literal attribute value option for an attribute with a restricted set of options.

What I like about this is the pi:publisher element can grow to be a better map of what a publisher file can have in it. It won't need to omit free-form variables.

Alex-Jordan commented 1 year ago

Force pushed with a cleaner logic.

Alex-Jordan commented 1 year ago

More thoughts on this one? I thought it was already merged, and went to continue folding more publisher variables into the scheme. But then I saw I was mistaken.

rbeezer commented 1 year ago

More thoughts on this one?

I like it. And in the queue. But have been deep on problem lists right now, and dynamic problems are calling me, too.

Put the reworked publisher items onto a follow-on commit?

Alex-Jordan commented 1 year ago

So it turns out there are not very many pubfile attributes that this can be used on right now without disrupting the status quo. One big reason is that many of them still support stringparams as something to look to when the pubfile option is not set. For example, html.knowl.theorem.

Is it a good time to stop supporting some of these stringparams?

Alternatively I could build support for a stringparam into the new scheme. So you'd have things like:

<xsl:variable name="knowl-theorem">
    <xsl:apply-templates mode="set-pubfile-attribute-variable" select="$publisher-attribute-options/html/knowl/@theorem">
        <xsl:with-param name="stringparam" select="$html.knowl.theorem"/>
    </xsl:apply-templates>
</xsl:variable>

Alex-Jordan commented 1 year ago

Rebased off master to stay current. And added one more commit that moved all (I think) of the simpler variables to this new scheme. "Simpler" excludes variables that are:

experimental
currently fall back to using a stringparam
manipulate the pubfile entry somehow when making the variable, as opposed to just using it
have some complicated selection logic

rbeezer commented 1 year ago

Thanks! I've got a list of manageable tasks I need to hit the next few days, and this is on it.

Alex-Jordan commented 1 year ago

I was looking at some of the variables that fall back to a stringparam, trying to think up a way to still honor the stringparam. But I think it's too hard to do in some nice way. It's not a matter of just using whatever the stringparam value was, although that would work in many cases. There can be some logic like the stringparamwebwork.inline.static is "yes" but that leads to the corresponding variable being "static". So, not straight up using the stringparam value.

Even worse are some that fall back to one srringparam, and if it's absent, use a different even older one.

Just thinking out loud, but we could pick an arbitrary amount of time (two years back, one year back, yesterday,...) and completely write off deprecations that are older than that.

rbeezer commented 1 year ago

A net loss of roughly 150 lines. ;-) Very good!

Two questions:

I always put the select first, and then the mode. Any reason for the opposite order? If not, I can silently switch them.
I'm not a fan of the formatting in the publisher file option tree, especially for things like a single short attribute, or no attributes. When there are four attributes and each has a list of three or four options, then spreading them out makes sense to me (like namespace aliases in stylesheets). I can edit here, too.

Alex-Jordan commented 1 year ago

Any reason for the opposite order?

I think it's more readable because of the mode values being aligned. The select value changes in length, but the mode value doesn't change. So this way, all the mode values are aligned and it's clear immediately they are all the same. The other way, you are tempted to verify that there is nothing special about a given line's mode value when you copy-paste it to make a new entry.

Not a big deal if you'd prefer it the other way.

And no problem to change the attribute formatting. I was just being consistent by default from one to the next. Less cognitive load that way.

rbeezer commented 1 year ago

If publication file has

<epub><cover/></epub>

or

<epub></epub>

or no such entry at all, then I get back

FileNotFoundError: [Errno 2] No such file or directory: '/home/rob/mathbook/mathbook/examples/epub/ext/*'

Does there need to be some logic that identifies a "wildcard" entry has not been set, so some default occurs? In this case we should be building a generic cover via the gauntlet. ;-) I guess we see $epub-cover-base-filename being blank (not *)? But I've not dug in too deep.

Alex-Jordan commented 1 year ago

Right. So there are these lines:

        <xsl:when test="string($pubfile-attribute) = ''">
            <xsl:value-of select="$default"/>
        </xsl:when>

But in this case, $default is a misnomer, and its value is '*'. We could split this up into two "when"s:

        <xsl:when test="string($pubfile-attribute) = '' and $default = '*'">
            <xsl:value-of select="''"/>
        </xsl:when>
        <xsl:when test="string($pubfile-attribute) = ''">
            <xsl:value-of select="$default"/>
        </xsl:when>

or something like that. Can you add it and try?

Alex-Jordan commented 1 year ago

Thinking better in the morning.

    <xsl:variable name="default" select="$options[1]"/>

should become

    <xsl:variable name="default" select="translate($options[1], '*', '')"/>

And then $default will be the right thing when the first option is *. (I'm assuming there will never be a valid option that is a string containing *.)

Alex-Jordan commented 1 year ago

Meant to add, should i try this and update the PR? Would it mess up some edits you already did with formatting?

rbeezer commented 1 year ago

No edits - saved those for after testing. ;-) Go ahead and experiment, and force-push if needed. i won't be able to get back to this right away.

Thanks.

Alex-Jordan commented 1 year ago

Is it safe to assume for now that * is never a character within a valid attribute string?

rbeezer commented 1 year ago

I think it is safe to assume there is no purpose for it, but if we are going to assume it is not ever there, then we should give a good warning when a publisher does happen to place one in an attribute.

rbeezer commented 1 year ago

Off the top of my head, while thinking about a different problem.

What if the "publisher tree" was a bit more verbose, so as to be more precise? Rather than attribute values leading to problems (bugs, assumptions) it looked like:

<webwork>
    <attribute name="server" freeform="yes" default="https://webwork-ptx.aimath.org">
    [several more here]
</webwork>

<latex>
    <worksheet>
        <attribute name="formatted" default="yes" options="no"/>
    </worksheet>
</latex>

We could model "switches" that have content (like LaTeX geometry, iirc) with a child "content" element that is a peer of attribute (not sure what else it would do).

Looking at the code, it might not take too much to reform defaults, options, paths, etc.

I'm liking this more and more as I think about it...

Alex-Jordan commented 1 year ago

For this purpose what matters is if there is a defined set of options, and the default one of those would be like abc*. What the publisher put in their file won't weigh in.

On Sat, Mar 11, 2023, 9:56 AM Rob Beezer @.***> wrote:

I think it is safe to assume there is no purpose for it, but if we are going to assume it is not ever there, then we should give a good warning when a publisher does happen to place one in an attribute.

— Reply to this email directly, view it on GitHub https://protect2.fireeye.com/v1/url?k=31323334-501d2dca-3132feb7-454455534531-8e21823e1a0358ac&q=1&e=c6f5ebca-66f7-4c55-bf2d-7231041f51cd&u=https%3A%2F%2Fgithub.com%2FPreTeXtBook%2Fpretext%2Fpull%2F1920%23issuecomment-1464964879, or unsubscribe https://protect2.fireeye.com/v1/url?k=31323334-501d2dca-3132feb7-454455534531-f7ffcbade6e972ff&q=1&e=c6f5ebca-66f7-4c55-bf2d-7231041f51cd&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABEDOAGH6IVPTTS5GOQNAD3W3S4E5ANCNFSM6AAAAAAUP4HMX4 . You are receiving this because you authored the thread.Message ID: @.***>

Alex-Jordan commented 1 year ago

I can't test building the epub sampler. I get:

$ ../../pretext/pretext -p publication.xml -c all -f epub-svg epub-sampler.xml 
Error: Cannot find module 'speech-rule-engine/lib/mathmaps/base.json'

Is that expected if I haven't set myself up for epub production? If it is a whole process to get that going, can you try out the suggested edit above to make

    <xsl:variable name="default" select="translate($options[1], '*', '')"/>

Right now the issue is from the clash between using a wildcard at all, with the convention that the first option is the default. The above edit will fix that.

Later I can think about your suggestion to make it more verbose (woke up with a feverish 8-yr old, so probably not coding much this weekend). First reaction: it would bring back more code verbosity than just inside that tree element. Because as things are now, it leverages a direct parallel between

$pubfile/latex/worksheet/@formatted

and

$publisher-attribute-options/latex/worksheet/@formatted

Your suggestion changes the latter to something like

$publisher-attribute-options/latex/worksheet/attribute[@name='formatted']/@default | $publisher-attribute-options/latex/worksheet/attribute[@name='formatted']/@options

I'm not sure that the individual assignments could be made with one-liners like the are now (<xsl:apply-templates mode="set-pubfile-attribute-variable" select="$publisher-attribute-options/latex/worksheet/@formatted"/>). But maybe. Or maybe not but it's worth losing that elegance.

rbeezer commented 1 year ago

OK, that edit allowed me to build the epub-sampler test document. But it will need more thorough testing before I embark on another full review. Which would mean getting yourself setup to build EPUB, which is pretty much a mainstream output format now.

OR,...

This is a feature for developers, so they should be more careful about reading directions. It'd be nice if we could assume that. ;-)

So I would much prefer a bomb-proof specification of the idealized tree, using XML syntax that we can assume every developer knows. As suggested earlier.

Using position (first is default), and special characters (* is a wild-card) is contrary to everything we have done with/for authors. So, even if some code becomes less elegant, I think the reliability is important enough to go this route. So I'd really like to see this PR go in that direction.

Alex-Jordan commented 11 months ago

I force pushed take 2 on this. Four new commits: 1 with infrastructure, then 3 that apply it to many existing variables. Those three commits are separated by how complex the variable selection was, with the idea that it might make testing easier. File drops by about 17%. The new machine is something like this:

Check if there is something like debug.chunk, a stringparam that overrides whatever may be in the pub file.
If the attribute is not declared in the pub file... a. if a deprecated stringparam is being used and its value is among the legal options, use that. b. if a deprecated stringparam is being used and its value is not among the legal options, use the default.
If the attribute is not declared in the pub file or declared to be null, use the default.
If the attribute is declared in the pub file and not null... a. if the attribute permits free form, use the given value b. if the given value is among the legal options, use it c. else use the default

Most of the time, the default is a string declared within pi:publisher but it could also be a dynamically determined default. There is a modal template to get the default.

Occasionally (like platform="aim") there is an option that has been retired. These are caught under 2b and 4c above, and a warning is issued.

Some deprecated stringparams previously have no validation check, and now with this, they do. So that's a change that may be bad. But also these are deprecated...

This is a lot for now. I see how to extend this to publisher "elements" like $publication/common/watermark for $watermark-text, but that can come later.

And there are some attributes that not only still support a deprecated stringparam (exercise.inline.statement), but also an even older deprecated stringparam (exercise.text.statement). I wanted to ask if I can apply the new machine to those and really kill off the older stringparam support.

There are some variables that prepend or append text to the string from the publisher file. Or do things like HTML encode space characters. I see how to handle these too with an extension of the machinery, but it's another thing for a future revision.

I tested with the sample article and sample book, using most of the provided publisher files for each. It's not comprehensive testing, but it passed (there was no unexpected diff).

Alex-Jordan commented 11 months ago

Maybe the commentary for each variable/pub attribute should move to near (or inside) the pi:publisher tree, so it's close to where things like default and options are actually defined. Thoughts?

Alex-Jordan commented 11 months ago

Force pushed with small things:

(1) added some commentary (2) simplified one piece of over-complicated logic at line 3119. (3) added commit to the end changing the set-pubfile-attribute-variable to set-pubfile-variable (in anticipation of the same mode applying to pubfile entries that are text, not attributes)

rbeezer commented 11 months ago

Just a note that I am looking over this one. As time allows...

Alex-Jordan commented 11 months ago

It's a lot to test :) The breakdown into commits might help with testing.

rbeezer commented 11 months ago

About to run through tests. Three cosmetic things I might do in a follow-on commit. Holler soon if they are not really cosmetic.

Modes get-default and path are too generic when these live in a global namespace (basically). I'd lengthen them to things like get-default-pub-attribute.
I'm consistent about @select first, with @mode second. And sometimes I rely on that for searches. So I'd switch these around.

rbeezer commented 11 months ago

Sample article with stock publication file. These appear to be variables that are not being expressed in the publication file.

PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.theorem" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@theorem  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.table" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@table  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.list" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@list  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.exercise.sectional" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@exercise-divisional  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "toc.level" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/common/tableofcontents/@level  from "0", "1", "2", "3", "4", "5", or "6".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.exercise.readingquestion" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@exercise-readingquestion  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.project" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@project  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.outcomes" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@outcomes  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.figure" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@figure  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.navigation.logic" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/navigation/@logic  from "linear" or "tree".
PTX:INFO    :     * PTX:WARNING: the stringparam "emdash.space" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/common/@emdash-space  from "none" or "thin".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.task" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@task  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "latex.pageref" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/latex/@pageref  from "yes" or "no".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.objectives" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@objectives  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.exercise.inline" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@exercise-inline  from "yes" or "no".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.navigation.logic" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/navigation/@upbutton  from "yes" or "no".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.listing" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@listing  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.definition" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@definition  from "no" or "yes".
PTX:INFO    :     * PTX:WARNING: the stringparam "html.knowl.exercise.worksheet" is deprecated. Also your value, "" is not a legal option. You should move to using a publisher file entry for  $publication/html/knowl/@exercise-worksheet  from "no" or "yes".

Alex-Jordan commented 11 months ago

Modes get-default and path are too generic when these live in a global namespace (basically). I'd lengthen them to things like get-default-pub-attribute.

This may extend to things in the pubfile that are not attributes. How about get-default-pub-variable?

I'm consistent about @select first, with @mode second. And sometimes I rely on that for searches. So I'd switch these around.

You can do you, of course. But it is easier for me to visually scan this code when the mode="set-pubfile-variable" and select="... are all aligned. If you swap them, the mode="set-pubfile-variable will lose alignment. Some part of my brain is going to want to spend more time looking at each line in the future.

Alex-Jordan commented 11 months ago

Sample article with stock publication file.

A last minute simplification in logic was a little too much oversimplifying. I'll let you know when I rebased wiith the patch back into the initial commit of this commit sequence.

Alex-Jordan commented 11 months ago

OK, back to working. Sorry for the hiccup.

rbeezer commented 11 months ago

OK, back to working.

Me, too. Thanks.

rbeezer commented 11 months ago

An artifact of testing. When there is a stringparam set AND there is a publisher file entry, then the publisher file entry "wins". Good. But we say "we will honor you selection". Can we detect both and so say "publisher entry wins!"?

Don't make a change now, just something to perhaps add very soon?

rbeezer commented 11 months ago

Scratch that, I think that is the deprecation warning...

rbeezer commented 11 months ago

PTX:WARNING: the publisher file  latex/@sides  entry should be "one" or "two", not "foo".  The default "" will be used instead.

The default value for this message should be from the get-default message, not from the @default attribute? (Two places, no?)

rbeezer commented 11 months ago

get-default template.

rbeezer commented 11 months ago

Stream-of-consciousness.

You should move to using a publisher file entry for  <xsl:value-of select="$full-path"/>  from <xsl:apply-templates select="$all-options" mode="quoted-list"/>.

"from" maybe makes sense in the code when a list comes next, but I'd write in documentation as

A publisher entry for ... with possible values, "...","...","..."

Alex-Jordan commented 11 months ago

Right, that one is this way in the tree:

<pi:pub-attribute name="sides" options="one two" legacy-stringparam="latex.sides"/>

No @default declared.

Technically they all use the get-default modal template. It's just that that template defaults (heh) to outputting @default. But for this one that is overridden to do something conditiional.

rbeezer commented 11 months ago

Should I fix it silently?

On June 5, 2023 11:36:58 AM PDT, Alex Jordan @.***> wrote:

Right, that one is this way in the tree:
<pi:pub-attribute name="sides" options="one two" legacy-stringparam="latex.sides"/>
No @.***` declared.

Technically they all use the get-default modal template. It's just that that template defaults (heh) to outputting @.***`. But for this one that is overridden to do something conditiional.

Alex-Jordan commented 11 months ago

We are miscommunicating... what is there to fix?

Alex-Jordan commented 11 months ago

Do you mean the wording in those deprecation warnings? If so, by all means change it to be more clear.

rbeezer commented 11 months ago

You set the variable to a default with the template, but you tell the publisher that you are going to use the value from the attribute. They aren't the same in the conditional cases.

On June 5, 2023 12:01:47 PM PDT, Alex Jordan @.***> wrote:

We are miscommunicating... what is there to fix?

Alex-Jordan commented 11 months ago

Can you print the exact warning message that you have in mind, as it appears in output?

Alex-Jordan commented 11 months ago

Two places, no?

Alright, I see where you mean. Earlier issue was up in the legacy-stringparam section and I was looking there.

Yes, please silently fix those two places. And there are two more. The warnings at 3114 and 3122 do not inform the publisher that a default will be used.

rbeezer commented 11 months ago

I decided to take responsibility for the proposed changes. ;-) Handling defaults, wording of error messages, some cosmetics.

Thanks for accomodating my attachment to this file. Its grown with a lot of work, and I'm glad it can keep growoing without so much work. Thanks.

rbeezer commented 11 months ago

Only one thing leftover from discussion, I think.

If you want to catch a legitimate pub-file entry AND any relevant string parameter use, and then say Pub entry wins, using "some-value" I think that would be a good enhancement, but I've not studied the logic necessary

Alex-Jordan commented 11 months ago

Thanks for the thorough review and testing, I'm sure it was a slog.

Yes, we can catch when there is a legitimate pub file entry as well as a stringparam and alert the publisher. Will do sometime soon in round 2. There are some more things for round 2:

And there are some attributes that not only still support a deprecated stringparam (exercise.inline.statement), but also an even older deprecated stringparam (exercise.text.statement). I wanted to ask if I can apply the new machine to those and really kill off the older stringparam support.

It would clean up a lot if the "Exercise component switches" templates could drop support for stringparams like exercise.text.statement, which is an older deprecation. Of course, still supporting the less old deprecation of stringparams like exercise.inline.statement. But do you think we should keep supporting them?

There are some variables that prepend or append text to the string from the publisher file. Or do things like HTML encode space characters. I see how to handle these too with an extension of the machinery, but it's another thing for a future revision.

I may try this too, and pass every pubfile entry through a "modification" template to possibly do string surgery.

rbeezer commented 11 months ago

I shoulda said today's discussion. ;-)

*.text.* parameters are coming up on the fifth anniversary of their death. An update of their deprecation message could be all that gets left behind.

Now totally ineffective, being ignored, you'll get a default from new scheme.
Move to the publisher file, skipping the intermediate stringparams.

rbeezer commented 11 months ago

I'd forgotten to check the (free form) EPUB cover! Seems to checkout OK.

PreTeXtBook / pretext

Pub attributes #1920