Open jfmengels opened 7 months ago
Parser.succeed (\_ y -> x) |= p
--> Parser.succeed (\y -> x)
needs to be
Parser.succeed (\_ y -> x) |= p
--> Parser.succeed (\y -> x) |. p
(because it will still consume ignored parsers)
And
Parser.mapChompedString (always f) p
--> p
should be
Parser.mapChompedString (always f) p
--> Parser.map f p
-- or alternatively (probably less nice in this context)
--> Parser.succeed f |= p
And maybe add
Parser.mapChompedString (\string parsed -> ..string, parsed..) (Parser.succeed a)
--> Parser.succeed (.."", a..)
Parser.succeed x |. p -> p
Parser.succeed () |. p -> p
succeed () |. a |. b |. c
should not simplify to a |. b |. c
(especially because the former scans better)p
is a Parser ()
. Before we get type inference, we can know it statically about chompIf
, chompWhile
, ...Parser.succeed (\_ y -> x) |= p -> Parser.succeed (\_y -> x)
As @lue-bird said, you need to keep the |. p
Parser.mapChompedString (always x) p -> Parser.succeed ()
This should read Parser.mapChompedString (always x) p -> Parser.succeed x |. p
(which may then be further simplified)
As @wolfadex mentioned:
Parser.succeed identity |= p -> p
, again only if we only have a single pipe
Parser.succeed x |. p -> p
is not a valid simplification. You'd lose the x and instead use result of p that was previously skipped. Maybe you meant |= instead of |.?
Thanks all for the feedback and corrections! Please double-check I didn't mess up anything.
@lue-bird I didn't get the last mapChompedString
example. Could you clarify it more?
@miniBill Good points about Parser.succeed x
needing to be ()
, I came to the same conclusion. We could potentially do it if we know that x
is the same as the value given by p
(Parser.succeed "abc" |. Parser.succeed "abc"), but that can be quite tricky. That said, I will add
Parser.succeed x |. Parser.succeed y-> Parser.succeed x
to the list.
@miniBill I've also changed the Parser.mapChompedString (always f) p
simplification to use Parser.map
instead. Let me know if you think that that is correct.
@Janiczek I don't think it works with |=
, because it would mean that x
has to be a function, likely meaning that it's doing something. I think the exception to that is Parser.succeed identity |= p
mentioned by @wolfadex and @miniBill (now added to the list).
I've also added Parser.succeed always |= p |= q
-> Parser.succeed identity |. p |= q`.
An example of how the mapChompedString
would trigger:
Parser.mapChompedString (\string parsed -> f string parsed) (Parser.succeed a)
--> Parser.succeed (f "" a)
because the string will always be ""
and the parsed value will be the same as the one given to succeed
.
So what we do is take the result of the lambda and replace the first arguments with ""
and the second arguments with a
.
This was raised in the Slack thread as well, but: I think the Parser.succeed () |. p -> p
and Parser.succeed identity |= p -> p
rules might be controversial and unwanted by some (me included) - go against a self-imposed "consistency" rule where most all parsers follow the shape
myParser =
Parser.succeed {- value or function -}
{- |. or |= -}
@Janiczek My idea (as suggested on Slack) is to only apply these changes "when there is only a single pipe in the expression"
Examples:
myParser0 =
Parser.succeed f
|. p
-->
myParser0 =
p
myParser1 =
Parser.succeed identity
|= p
-->
myParser1 =
p
-- Unchanged
myParser2 =
Parser.succeed f
|. p
|= q
Would this not suit you? If so, do I understand correctly that you prefer:
myParser4 =
Parser.succeed identity
|= Parser.token "/"
-- over
myParser4 =
Parser.token "/"
I'd say the latter is always a simplification over the former. Or are there other scenarios that you're thinking of?
A common pattern I tend to use is to use the "identity" part as a description, e.g.
Parser.succeed (\name size -> File { name = name, size = size })
|. ...
|. ...
|= ...
|. ...
|. ...
|= (Parser.succeed (\size -> size)
|= Parser.int
)
|. ...
without the explaining identity function, it gets pretty hard to read as you have to jump around the code to know what you're actually producing.
Also makes it easy to add |.
s to just the file size parser in the future, or even upgrade it to e.g. a single-variant type.
To be fair nobody does e.g. Json.Decode.map (\fieldName -> fieldName) fieldJsonDecoder
so count my opinion as "this simplification is probably fine".
As @lue-bird showed, having the un-simplified succeed
-ful expression makes it easier to add |.
or |=
lines before / after the parsed thing, and has some value, even though having p
on a line by itself is a simplification.
It's probably fine, it's just stylistic, but I know you do pay attention to these stylistic issues in some rules ("why would somebody not use this") and this is potentially one of them. The other rules seem non-controversial.
@lue-bird Would a comment not serve the same purpose?
Parser.succeed (\name size -> File { name = name, size = size })
|. ...
|. ...
|= ...
|. ...
|. ...
-- Size
|= Parser.int
|. ...
Or by extracting it and giving it a name like |= sizeParser
.
Also makes it easy to add
|.
I absolutely agree with this. But I'd say a lot of simplifications already destroy this. For instance (and I know it's a bit odd, but for the sake of example), one could want to keep List.map identity list
because that would make it easier to switch to List.map f list
than if you only had list
. I think that both this rule and the unused
rules show that a lot of the cases where you want to keep something written in a way that makes it easier to retrieve/edit later doesn't matter much.
I'd be more in favor to not apply the simplification if there were some readability benefits.
it's just stylistic, but I know you do pay attention to these stylistic issues in some rules
@Janiczek Absolutely. I'm mostly trying to figure out whether there is a better approach that we can all agree on is better. If there is none or if there is no consensus, then I am very happy to drop this 😄 Just to explicit it, all of my questions are genuine and come out that search and of my own curiosity.
I guess where I come from here is that I find it hard to believe that
Parser.succeed identity -- or with lambda
|= Parser.int
is easier to read than just Parser.int
(eventually with a comment or extracted as a variable).
I've only found one example in my own code:
Parser.succeed (\right -> BinaryOp left RangeInclusive right)
|> Parser.keep (Parser.lazy (\() -> exprAux precedence isRight))
I think I'd be OK with changing it into
Parser.lazy (\() -> ...)
|> Parser.map (\right -> ...)
if elm-review suggested that, even though it's not how I'd have written things... :+1:
@Janiczek I'm confused, I don't think your example is one of the cases above? :thinking:
@miniBill You can imagine it being
Parser.succeed identity
|> Parser.keep (Parser.lazy (\() -> exprAux precedence isRight))
changing into
Parser.lazy (\() -> ...)
The latter is definitely something we should do, but your example with Parser.succeed (\right -> ...
I think we shouldn't touch
Parser.succeed always |= p |= q
->Parser.succeed identity |. p |= q
needs to be
Parser.succeed always |= p |= q
-> Parser.succeed identity |= p |. q
(|=
and |.
are swapped)
since it's equivalent to
Parser.succeed (\a _ -> a) |= p |= q
, not Parser.succeed (\_ a -> a) |= p |= q
.
Parser.map
simplificationsParser.andThen
simplificationsParser.succeed () |. p
->p
p
is aParser ()
Parser.succeed identity |= p
->p
Parser.succeed x |. Parser.succeed y
->Parser.succeed x
Parser.succeed always |= p |= q
->Parser.succeed identity |. p |= q
Parser.succeed (\_ y -> x) |= p
->Parser.succeed (\y -> x) |. p
(Note, may need import of the|.
operator)Parser.chompIf (always False)
->Parser.succeed ()
Parser.chompWhile (always False)
->Parser.succeed ()
Parser.mapChompedString (always f) p
->Parser.map f p
Parser.getChompedString (Parser.succeed x)
->Parser.succeed ""
As far as I can tell, all of these would also apply to
Parser.Advanced
.