WIP: First sketch of parametrizing the AST (followup on #148)#149
WIP: First sketch of parametrizing the AST (followup on #148)#149VictorCMiraldo wants to merge 4 commits intoDaniel-Diaz:masterfrom
Conversation
Text/LaTeX/Base/Render.hs
Outdated
| renderBuilder (ParArg l) = "(" <> renderBuilder l <> ")" | ||
| renderBuilder (MParArg []) = mempty | ||
| renderBuilder (MParArg ls) = "(" <> renderCommasBuilder ls <> ")" | ||
| instance (Show a) => Render (TeXArg a) where |
There was a problem hiding this comment.
Why is Show needed here?
There was a problem hiding this comment.
I asked myself the same question, it comes as a superclass of Render, which is delcared as:
class (Show a) => Render a ...There was a problem hiding this comment.
Interesting. Maybe that's something to overthink independently... I suspect this might be an artifact from the time when Show was a superclass of Num, or something silly like that.
There was a problem hiding this comment.
Interesting. Maybe that's something to overthink independently... I suspect this might be an artifact from the time when
Showwas a superclass ofNum, or something silly like that.
It was in order to have a default method that uses show. Maybe not the best idea in restrospect...
There was a problem hiding this comment.
@Daniel-Diaz
-XDefaultSignatures?
Yes. It could be changed to use that. But currently I think show is the wrong function for this use case, as show is supposed to generate Haskell code that can be used to produce the value back. However, removing the Show superclass now might break code that uses the default implementation. So default signatures is probably the best way to go.
There was a problem hiding this comment.
It might still break some code that relies on the superclass, but such code perhaps should be broken.
There was a problem hiding this comment.
I agree that show is the wrong function for the use case.
Text/LaTeX/Base/Syntax/WithParm.hs
Outdated
|
|
||
| -- | Type of @LaTeX@ blocks. The @a@ type variable can be used for | ||
| -- a number of things but mainly stores source location. | ||
| data LaTeX a = |
There was a problem hiding this comment.
Does it really make sense to stuff the a parameter in every single constructor? That seems excessive.
In fact, how about going about it the opposite way: we could introduce just a single, new constructor, that's responsible for nothing else but position information etc., and is included in the AST through TeXSeq. The “no positions mode” would be LaTeX Void instead of LaTeX ().
There was a problem hiding this comment.
I agree it seems a little excessive, but I'm also unsure about the single constructor.
Lets say we add a single constructor, call it TeXLoc, now
Imagine that we are using locations for the sake of error reporting, and we are currently processing a TeXEnv constructor: if we wish to raise an error and indicate to the user where the error came from in the source file, we'd have to find the closest TeXLoc and hope its accurate enough.
I'll think this through a little more.
There was a problem hiding this comment.
@leftaroundabout I have removed the src location from most constructors; now only TeXRawL, TeXCommSL, TeXCommL, TeXEnvL and TeXMathL carry source location (as indicated by their L suffix). The TeXEnvL carries two locations corresponding to where the respective \begin and \end are.
I also changed the parser to add such annotations.
Maybe this is a good middle ground. curious to hear what you think.
Cheers,
V
| import Data.String(IsString(fromString)) | ||
|
|
||
| import Text.LaTeX.Base.Class(LaTeXC, comm0, comm1, comm2, liftL, liftL2) | ||
| import Text.LaTeX.Base.Syntax(LaTeX(TeXComm, TeXEnv), TeXArg(FixArg, OptArg)) |
There was a problem hiding this comment.
Maybe this module, and the other Package ones, could simply be changed to not rely on plain constructors in the first place. Then there wouldn't be a need for fiddling around with pattern synonyms.
3 participants
I just made a quick sketch of parametrize the AST with data that would support the addition of source location tokens (#148). The libary builds fine and the tests pass. 👍
I don't necessarily like the amount of pattern synonyms in
Text.LaTeX.Base.Syntaxnor the fact that to import non-annotated AST variants one would have to use-XPatternSynonymsand writepatternexplicitly. For example, inText.LaTeX.Packages.Acronymthe import line:Would have to become:
Instead, I just imported the whole
Syntaxmodule:import Text.LaTeX.Base.Syntax