HaTeX icon indicating copy to clipboard operation
HaTeX copied to clipboard

Proposal: Remove TeXSeq and TeXEmpty

Open imeckler opened this issue 10 years ago • 6 comments

Is there a compelling reason for the TeXSeq and TeXEmpty constructors vs. something like

newtype LaTeX = LaTeX [Block]
data Block =
   TeXRaw Text
 | TeXComm String [TeXArg]
 | TeXCommS String
 | TeXEnv String [TeXArg] LaTeX
 | TeXMath MathType LaTeX
 | TeXLineBreak (Maybe Measure) Bool
 | TeXBraces LaTeX
 | TeXComment Text
   deriving (Eq,Show,Typeable)

The advantage of this over the current type is that it seems to better reflect the meaning of a LaTeX document. Specifically, it removes a lot of terms which denote the same TeX expression but have different structures. E.g., currently

TeXSeq (TexSeq a b) c /= TeXSeq a (TeXSeq b c)

and

TeXSeq TeXEmpty TeXEmpty /= TeXEmpty

but these distinctions are not expressible in the type I propose. I suppose with the current scheme one gets O(1) concatenation, although the cost of converting the tree into a list is likely to be paid during conversion to text, so it's unclear if this is an advantage.

My personal reason for wanting to make this change is to make it easy to parse a LaTeX document using parsec. Currently I have to implement a nontrivial stream instance which is only correct assuming the document has the form

TeXSeq a1 (TeXSeq a2 .(TeXSeq a3 ...) ... )

with the constructor of ai not equal to TeXSeq. This assumption holds when using output from the library's parser, but it would be nice if the types forced this condition to hold.

imeckler avatar Nov 13 '14 23:11 imeckler

I have delayed answering this issue because I didn't have much time and the proposal does require attention.

First of all, in my opinion, you are arising a valid point here. In the other hand, it is clear that this is a breaking code change, so it must be treated carefully. Plus, the behavior you want can be obtained using (<>) instead of TeXSeq (which is recommended in the docs). Of course, this is not as clean as just forcing that behavior with types.

The trick in the Monoid instance would dissappear, which is something I like. I can imagine this would imply some other changes throughout the library. Maybe an actual pull request would clarify the changes we are heading to. Are you open to that? If not, I will write it myself, but it'll take a little longer. Anybody else can see any other downsides besides backwards code compatibility? Will creating new functions be any more tedious?

Daniel-Diaz avatar Nov 18 '14 21:11 Daniel-Diaz

@Daniel-Diaz The trick wouldn't be nessesary, the associativity would be trivial by the list monoid.

NightRa avatar Nov 19 '14 05:11 NightRa

@NightRa That's exactly what I meant. Sorry if it wasn't clear.

Daniel-Diaz avatar Nov 19 '14 14:11 Daniel-Diaz

@Daniel-Diaz I'll start working on a pull request. I'm somewhat busy at the moment, so I'll let you know if it seems like I won't be able to finish it in a timely manner.

imeckler avatar Nov 19 '14 15:11 imeckler

@Daniel-Diaz I just forked and started updating modules in dependency order. I've done Text/LaTeX/Base/Class.hs Text/LaTeX/Base/Render.hs Text/LaTeX/Base/Syntax.hs Text/LaTeX/Base/Texy.hs Text/LaTeX/Base/Warnings.hs and am now on Text/LaTeX/Base/Commands.hs, but the changes in this file are pretty tedious, so I wanted to see what you thought before going ahead.

imeckler avatar Nov 19 '14 16:11 imeckler

The commit mentioned by @imeckler : https://github.com/imeckler/HaTeX/commit/aa5f7cdb29b6aded47ec376268487659c924ffa0

Daniel-Diaz avatar Nov 19 '14 18:11 Daniel-Diaz