Extend from Parser

[TimoMeijer]

Extending from Parser adds some convenience methods to the Parser out of the box, such as decodeAccumulating.
The parser package object already matches the Parser interface.

Relates to #199

[TimoMeijer]

@zmccoy @jeffmay Would you have time to take a look? Thanks!

[SethTisue]

(As an aside, I see that readme in this repo still names Travis as a maintainer.)

[zmccoy]

Thanks for doing this!

[zmccoy]

@TimoMeijer A preemptive approval when I thought I saw it green on my part. So it looks like things run fine on 2.11, and 2.12. On 2.13 however, when extending Parser on the package object it compiles, but ends up acting as if the def parse(yaml: Reader): Either[ParsingFailure, Json] definition doesn't exist and that the method that takes a string is the only one. Changing names would break bincompat, and I'm honestly not sure if this is a bug in the 2.13 compiler or an expected change yet.

[TimoMeijer]

I can see if I can reproduce this locally tomorrow, and if so, investigate further

[TimoMeijer]

The compile error actually occurs in both 2,12 and 2.13, but not in 3.
If I change the package object into a regular object, the error disappears.
I have not yet figured out why this is the case, but I'll do some more research.

[TimoMeijer]

Minimal example of the behavior: https://gist.github.com/TimoMeijer/bef21c11cb13fa283205de1dd32347d2

[zmccoy]

That looks to track with what we found when we were looking at things, I missed the 2.12 failure though but that makes sense.

[TimoMeijer]

@zmccoy Would it make sense to split the implementation of parser between 2.x and 3.x, as we can support this behavior on 3.x already? Or would we not want to implement this, and have the inconsistency between circe and circe-yaml until 2.x support is dropped? Or check if there might be any workarounds?

[jeremyrsmith]

Sorry, I know I don't maintain this anymore. But I want to ask:

Do you want to extends Parser so that this package can be substituted for a Parser? Or is it just to inherit the < 20 lines of code that Parser gives you?

I'd hope it's not the former, because you can't substitute a JSON parser for a YAML parser (nor vice versa, necessarily).

I'd hope it's not the latter, because those methods aren't special enough to couple these.

Just my opinion as an ex-maintainer.

[TimoMeijer]

Thanks for your thoughts @jeremyrsmith!

My original case was that I wanted to use decodeAccumulating with yaml files. On looking into it further, I noticed it was implemented on the Parser trait in circe core, which the circe builtin jawn parser module implements. It seemed logical that the yaml parser should be implemented similarly, and inherit the same functionality and convenience methods.

You're of course completely right that simply copying over decode and decodeAccumulating would add the same
convenience methods. Personally, I would argue that extending Parser would be a more future-proof implementation, over duplicating the convenience methods, as it would ensure circe parser and circe-yaml parser methods stay inline. Although this compiler bug makes that choice less straightforward.

@jeremyrsmith, is your point that we should add these convenience methods, but not through extending from Parser, or that we shouldn't add these conveniences at all?

[jeremyrsmith]

@TimoMeijer I guess my point is that, in Parser, the desired method decodeAccumulating is just composing parse and then the decoder's decode. The details of composing an Either[E, T] with a T => ValidatedNel[E, T] are not likely to change, so extending an interface just to inherit that well-defined method might not be worth the trouble it's causing.

Another solution would be to pull out the implementation in question to a function. Would that seem silly for the method in question?

Again, sorry to butt in here. Not trying to imply I have answers.

But extending Parser might be an XY problem @zmccoy 😉

[zarthross]

Whops, sorry, was working on an alternative and accidentally pushed into master... I've fixed that, this isn't really merged in.. 😞

[zarthross]

@TimoMeijer Thoughts on #338?

[TimoMeijer]

Seems like a clean workaround! On 4 Nov 2022 22:58, Darren Gibson ***@***.***> wrote: @TimoMeijer Thoughts on #338? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>