Fix deltaparsing #1197
Fix deltaparsing #1197
Conversation
src/Pact/Compile.hs
Outdated
|
|
||
| moduleState :: Compile ModuleState | ||
| moduleState = use (psUser . csModule) >>= \m -> case m of | ||
| moduleState = use (psUser . csModule) >>= \case |
There was a problem hiding this comment.
This PR is doing so much more than fixing delta-parsing. Can you submit all of these minor cleanups in their own PR first? That way we can trivially approve and merge those, and then think harder about just the Delta parsing fix.
There was a problem hiding this comment.
I can, it just is quite a bit of extra work to create new PR each time I fix syntax. There are just so many of those syntactic inconsistencies that make readability of the code unnecessarily hard. Fixing them from time to time along the way is just easier.
There was a problem hiding this comment.
In this case a LOT of the changes are just fixing syntax, so then it makes sense to bundle all of that up into a PR before making an important change like delta parsing.
There was a problem hiding this comment.
(The issue is that I don't first read the the whole code base and clean it up and then start working on the feature, but those changes are interleaved with other changes in the PR. For instance, above PR caused merge conflicts during the cherry pick and now I have to wait for it to be squash merged into master before I can merge it back into the original PR, which again will require to resolve the conflict.)
* tweaks to deltaparser * Update tests/GasModelSpec.hs --------- Co-authored-by: Lars Kuhtz <lars@kadena.io>
| @@ -47,7 +47,7 @@ flag tests-in-lib | |||
| manual: True | |||
|
|
|||
| library | |||
|
|
|||
| cpp-options: -DLEGACY_PARSER=1 | |||
There was a problem hiding this comment.
A couple thoughts
You don't need to set =1 for binary flags, you can just define -DLEGACY_PARSER.
Then CPP if-blocks become like
#ifdef LEGACY_PARSER
...
#else
...
#endifor
#if someCondition
...
#elifdef LEGACY_PARSER
...
#endifThe other thought is that it could be behind a flag:
flag legacy-parser
description: ...
default: True
manual: True
library
if flag(legacy-parser)
cpp-options: -DLEGACY_PARSERWhich would make toggling it on/off not require editing the cabal file.
There was a problem hiding this comment.
Agreed, this is all temporary for now. I tried to define the macro in cabal.project.local but cabal doesn't like that, so I had to put it into the cabal file.
Regarding using 1/0, sometimes I like to be more explicit when dealing with macros (I find it easy to mess up with #define and #undef). Yeah, if that macro would still be in the final version of the code, we would use a standard pattern with (automatic) cabal flag. But, hopefully, that won't be necessary.
| -- hidden chars like Trifecta). | ||
| -- | ||
| instance DeltaParsing PactAttoparsec where | ||
| line = return mempty |
There was a problem hiding this comment.
What about something like this:
| line = return mempty | |
| line = PactAttoparsec (fail "DeltaParsing.line is not supported for PactAttoparsec") |
The error message is more informative, than giving an empty string.
There was a problem hiding this comment.
assuming it's actually not used ...
| {-# INLINE position #-} | ||
|
|
||
| slicedWith f a = (`f` mempty) <$> a | ||
| rend = return mempty |
There was a problem hiding this comment.
Similar to line, I think rend and restOfLine, if not used, should return informative error messages
Goal of this PR is not necessarily to deploy a fix for deltas in the production parser. The goal is to document the current and correct behaviors and provide implementations for correct and legacy behavior. This will help to explore the current state of affairs and future fixes.