WIP - An alternative approach to #31 (see also #33) #34
Conversation
This approach moves the node handling to an object algebra. This allows for relatively easily adding configurable stuff that's able to change how various YAML types are handled as the need arises. One can even plug in their own algebra if they want to get nitty-gritty. It also allows adding optional error accumulation, which is something that's always felt missing. A nice thing here is by default we don't have to even check any configuration - only when a parser instance is configured do we change to `ConfiguredAlg`. Also should be binary compatible (and `yaml.parser.configure(...)` is a nice way to get a configured parser!). TODO: Add tests around error accumulation and such (coverage currently lacking)
| } | ||
| } | ||
|
|
||
| final class AccumlatingAlg[A](base: NodeAlg[A]) extends NodeAlg[ValidatedNel[ParsingFailure, A]] { |
There was a problem hiding this comment.
Nitpick but is the missing u intentional?
| final override def timestamp(node: ScalarNode): Json = if (!numericTimestamps) { | ||
| super.timestamp(node) | ||
| } else { | ||
| val constructor = new SafeConstructor.ConstructYamlTimestamp() |
There was a problem hiding this comment.
Some part of me doesn't really like the idea of instantiating a constructor for every single timestamp node (especially since it involves a non-trivial amount of setting up its own internal state), but it probably doesn't matter much.
There was a problem hiding this comment.
Because of that mutable internal state, I'm not sure what the other option is. You're not guaranteed that the algebra isn't being used from multiple threads.
There was a problem hiding this comment.
Unfortunately they don't expose just the timestamp parsing logic - otherwise we could bypass this allocation entirely.
Since the spec is pretty fixed, maybe it's worthwhile just duplicating their regexes and doing it internally here?
| node.getValue.asScala.map { | ||
| nodeTuple => nodeTuple.getKeyNode match { | ||
| case keyNode: ScalarNode => keyNode.getValue -> any(nodeTuple.getValueNode) | ||
| case _ => throw ParsingFailure("Only string keys can be represented in JSON", null) |
There was a problem hiding this comment.
The null isn't new but it makes me a little uncomfortable—so far in circe I haven't needed ParsingFailures that don't wrap an underlying exception, but this seems like a reasonable thing to want in this case, so maybe that member should be an Option.
There was a problem hiding this comment.
I could just give it an actual exception here. I don't think you should change ParsingFailure.
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
===========================================
- Coverage 100% 72.43% -27.57%
===========================================
Files 4 5 +1
Lines 106 156 +50
Branches 3 7 +4
===========================================
+ Hits 106 113 +7
- Misses 0 43 +43
Continue to review full report at Codecov.
|
|
@jeremyrsmith What do you think about moving forward with this? I can live with the constructor construction. |
Working on getting coverage back to 100%
|
Apologies for letting this sit for multiple years. I think #31 solved the base issue – though I guess #33 was closed in favor of this PR (and I failed to shepherd this PR adequately 😞 to the point where it's beyond stale). Anyway, closing this one to avoid polluting the open issues; if anybody feels like it solves anything, feel free to reopen it and I'll try and figure out what I was doing with it 3 years ago and see if anything there is still applicable. |
This approach moves the node handling to an object algebra. This allows for relatively easily adding configurable stuff that's able to change how various YAML types are handled as the need arises. One can even plug in their own algebra if they want to get nitty-gritty.
It also allows adding optional error accumulation, which is something that's always felt missing.
A nice thing here is by default we don't have to even check any configuration - only when a parser instance is configured do we change to
ConfiguredAlg.Also should be binary compatible (and
yaml.parser.configure(...)is a nice way to get a configured parser!).TODO: Add tests around error accumulation and such (coverage currently lacking)