search

Daniel-Diaz / HaTeX

The Haskell LaTeX library.
BSD 3-Clause "New" or "Revised" License
199 stars 46 forks source link

Proposal: Remove TeXSeq and TeXEmpty #40

Open imeckler opened 9 years ago

imeckler commented 9 years ago

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.

Daniel-Diaz commented 9 years ago

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?

NightRa commented 9 years ago

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

Daniel-Diaz commented 9 years ago

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

imeckler commented 9 years ago

@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 commented 9 years ago

@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.

Daniel-Diaz commented 9 years ago

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