Conversation
src/Formula/Printing.hs
Outdated
| where | ||
| extraText onNo _ NoExtraText = onNo | ||
| extraText _ func (Static s) = func s | ||
| extraText _ func (Collapsible _ s _) = func s |
There was a problem hiding this comment.
This looks suspicious. Information is being lost here, and then cannot be reconstructed in src/Formula/Parsing.hs. The printing and parsing should have a roundtrip property (printing an instance and then parsing it gives the back the original instance), which requires injectivity of the behavior here.
| bonusText <- option NoExtraText (tokenSymbol "," *> (parser :: Parser ExtraText)) | ||
| char ')' | ||
| pure $ PickInst cs (read index) (read printSol) (fromList . read <$> bonusText) | ||
| pure $ PickInst cs (read index) (read printSol) bonusText |
There was a problem hiding this comment.
I‘m now wondering why all this parsing and pretty-printing work is happening in this repo at all.
@patritzenfeld, shouldn‘t this be handled on the Autotool side, using Reader and ToDoc instances, instead? That would likely obviate much of the code in the Parsing and Printing modules here.
There was a problem hiding this comment.
These instances for PickInst are indeed not required. This is an ancient relic from my bachelor's thesis. Deriving ToDoc and Reader in Autotool should provide the same functionality (and ExtraText is already an instance of those classes as well).
So all of the printing and parsing code for PickInst and ExtraText here can be removed.
There was a problem hiding this comment.
I guess PickInst is not the only type to which this applies, then?
Maybe there should be a separate pull request which removes (almost?) all of Parser.hs and Printing.hs, coordinated with corresponding changes in Autotool when calling the affected logic-tasks task types.
That other pull request could then be merged before this one here.
There was a problem hiding this comment.
Well, some of the types will still need custom instances instead of derived ones (mostly student input related). Since we now have the required Autotool libraries already in the stack.yaml, we could also define these Reader and ToDoc instances in this repo, just like modelling-tasks does.
In that case, instead of deleting the code, it could just be adapted slightly.
There was a problem hiding this comment.
Since we now have the required Autotool libraries already in the stack.yaml
I don't think that's true.
Let's make the minimal change here (in a separate PR): Remove all the parsing and printing that could instead be done with Reader and ToDoc instances in Autotool. Why that's not all of them, I don't yet understand, i.e., what student input has to do with it or prevents simply makinge all of the instances being written and parsed with just the generic/derived instances.
3 participants
CLOSE #281