sile-typesetter / sile

The SILE Typesetter — Simon’s Improved Layout Engine
https://sile-typesetter.org
MIT License
1.61k stars 97 forks source link

Fix spelling mistakes in simpletable package documentation #2034

Closed leorosa closed 1 month ago

leorosa commented 1 month ago

This PR fixes the simpletable package documentation.

But I would also ask your opinion on the following: today this package must be called from a class, and calling it from a document raises an error. I think that this behavior is not intentional, given the tests present in line 15: if type(options) ~= "table" or pl.tablex.size(options) <= 3 then When simpletable is called from a document, it receives a table as options, and its size is 12. Thus, the check in line 15 fails. What do you think, to change the second test to if type(options) ~= "table" or pl.tablex.size(options) ~= 3 then ?

Omikhleia commented 1 month ago

But I would also ask your opinion

Personally, I think "simpletable" should be killed :)

Omikhleia commented 1 month ago

But more seriously, their should be a way to pass package options on the \use command, and that might have been the idea here... (although it raises a bunch of additional concerns -- so one would have to load multiple instances of the package to get several tags working? Sounds somewhat defective by design.

leorosa commented 1 month ago

Personally, I think "simpletable" should be killed :)

I understand, and kind of agree with you. But, since it is the only way available to produce a table, I think that it should work, as simple as it is :)

But more seriously, their should be a way to pass package options on the \use command, and that might have been the idea here...

I have a feeling that the code, as it is, 'used' to work in earlier versions.

Omikhleia commented 1 month ago

But, since it is the only way available to produce a table, I think that it should work, as simple as it is :)

Well, out-of-box it's the only way, but there's at least an alternative available, even mentioned in the Wiki: my ptable.

Or to phrase it differently: how long will SILE keep subpar implementations of that sort in its code base?

But you are right that it's not a reason not to fix it for now indeed... though eventually a good deprecation would probably be better.

alerque commented 1 month ago

Re @leorosa...

today this package must be called from a class, and calling it from a document raises an error

What are you refering to here? I don't see any problem loading this package from a document:

\begin{document}
\use[module=packages.simpletable]
\begin{table}
\begin{tr}
\td{foo}\td{bar}\td{baz}
\end{tr}
\end{table}
\end{document}

Re @Omikhleia...

But more seriously, their should be a way to pass package options on the \use command

There is.

\begin{document}
\use[module=packages.simpletable,tableTag=difftable,trTag=difftr,tdTag=difftd]
\begin{difftable}
\begin{difftr}
\difftd{foo}\difftd{bar}\difftd{baz}
\end{difftr}
\end{difftable}
\end{document}

I tend to agree this module needs to get deprecated and a more robust default table implementation should come standard, but I don't follow along at all with the supposed issue here.

leorosa commented 1 month ago

Re @alerque What are you refering to here? I don't see any problem loading this package from a document:

Thanks for the answer. I just realized that a program of mine was outdated. For the record, the offending line was:

\script[src=packages/simpletable]

And I had the feeling that the simpletable code used to work in earlier versions... oh, the irony. Feel free to close this issue!

Re @Omikhleia... Well, out-of-box it's the only way, but there's at least an alternative available, even mentioned in the Wiki: my ptable.

Yes, I was referring to an out-of-the-box way. Thanks for the suggestion; I already knew that your ptables existed. I'll give it a try.

alerque commented 1 month ago

Re @alerque Thanks for the answer. I just realized that a program of mine was outdated. For the record, the offending line was:

\script[src=packages/simpletable]

This usage of script is definitely deprecated. The short story is just use \use instead.

The \script implementation has always been riddled with hacks and we're trying to clean them up. The reason it isn't loading simpletable correctly could easily be fixed (I just tried it is a one liner) but fixing it also breaks a lot of other legacy usage out there that is using the weird current behavior that passes a copy of the class instead of the command options. It's clearly wring, but fixing it would break a bunch of documents. The way to fix those documents is to switch to use anyway, so I think we'll just leave it be.

And I had the feeling that the simpletable code used to work in earlier versions... oh, the irony. Feel free to close this issue!

The current implementation could not possibly have supported simpletable since at least 589a1564. It probably did sometime before that.

Given that \use works properly now just move on with that.

Yes, I was referring to an out-of-the-box way. Thanks for the suggestion; I already knew that your ptables existed. I'll give it a try.

The ptable package is quite useful, I use it a bit myself.

Omikhleia commented 1 month ago

@alerque

There is (followed by example)

Wow, nice! We should perhaps add that example to the doc maybe?

... but I don't follow along at all with the supposed issue here.

I am not sure what is "the supposed issue here" (or if I had any, beyond the point :clown_face: that "simpletable" as implemented is somewhat "subpar").

alerque commented 1 month ago

Wow, nice! We should perhaps add that example to the doc maybe?

Yes, probably should, although we already document \use for loading a package and that just works out of the box.

I am not sure what is "the supposed issue here" (or if I had any, beyond the point 🤡 that "simpletable" as implemented is somewhat "subpar").

Originally I was baffled by this:

this package must be called from a class, and calling it from a document raises an error

I couldn't replicate it at all hence calling it "supposed". In the end that turns out to be because I was trying the replication with \use. It turns out it does raise an error if you try with \script.