Add module that uses snakeyaml-engine, which only handles yaml 1.2 #303
Conversation
|
Note that i made that commit two minutes after I got it to compile. I gave also never used circe-yaml or snakeyaml before. As such I'd expect it will take some work to finish it. Tests are not green. Also this seems to be breaking public api, and a breaking change to yaml format. This should at the minimum get a new major version, perhaps it should be a separate artifact, or even a separate library. |
|
Maybe we could find a way to do this that does not break the public API. |
|
I'd say the opposite, let's make it very obvious and clear. This update can and will interpret arbitrary yaml differently Also the new snakeyaml library is very configurable with config objects. Let's rather use those, so for instance the printer can become much simpler. The only thing we really need to provide is a Node <-> JSON bijection and some syntax |
|
Related issue on BitBucket: snakeyaml-engine Cannot parse integers encoded in hexadecimal |
Codecov ReportBase: 87.30% // Head: 87.35% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #303 +/- ##
==========================================
+ Coverage 87.30% 87.35% +0.05%
==========================================
Files 5 14 +9
Lines 126 253 +127
Branches 9 13 +4
==========================================
+ Hits 110 221 +111
- Misses 16 32 +16
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Hi @jeffmay , would you like to merge this yourself of can I do it? |
| def flatten(node: MappingNode): MappingNode = { | ||
| flattenMapping(node) | ||
| node | ||
| } | ||
|
|
||
| def construct(node: ScalarNode): Object = | ||
| getConstructor(node).construct(node) | ||
| super.construct(node) | ||
| } | ||
|
|
||
| private[this] def yamlToJson(node: Node): Either[ParsingFailure, Json] = { | ||
| // Isn't thread-safe internally, may hence not be shared |
There was a problem hiding this comment.
Does this thread-safety comment still apply?
There was a problem hiding this comment.
I don't know, to be honest. And since I don't know, I'm picking the conservative option of keeping things as they were.
| } | ||
|
|
||
| private[this] def yamlToJson(node: Node): Either[ParsingFailure, Json] = { | ||
| // Isn't thread-safe internally, may hence not be shared | ||
| val flattener: FlatteningConstructor = new FlatteningConstructor | ||
| val flattener: FlatteningConstructor = new FlatteningConstructor(settings) |
There was a problem hiding this comment.
See the if node.getValue.startsWith("0x") || node.getValue.contains("_") check below - snakeyaml-engine doesn't seem handle hexadecimal-formatted numbers as far as I can tell. Those just get tagged / handled as string.
There was a problem hiding this comment.
Not now, but maybe in the future
https://bitbucket.org/snakeyaml/snakeyaml-engine/issues/38/cannot-parse-integers-encoded-in
| if (ot.isPresent) Some(ot.get()) else None | ||
|
|
||
| private[this] def createComposer(reader: Reader) = | ||
| new Composer(settings, new ParserImpl(settings, new StreamReader(settings, reader))) |
There was a problem hiding this comment.
new Compose(settings).composeReader(reader)There's a util method that defines all this + the .getSingleNode call in the form of Compose.composeReader
There was a problem hiding this comment.
But that returns Optional[Node] instead of a Composer...
|
I wasn't able to implement all the suggestions from the comments, so I've at least explained the reason in the comments. Otherwise this is ready to be merged @jeffmay |
sideeffffect
changed the title
|
I'll rework this into 2 separate modules, one for 1.1, the other for 1.2. |
sideeffffect
force-pushed
the
snakeyaml-engine
branch
3 times, most recently
from
c9d9707
to
f15ce82
Compare
sideeffffect
force-pushed
the
snakeyaml-engine
branch
from
f15ce82
to
e8f01d4
Compare
sideeffffect
changed the title
sideeffffect
force-pushed
the
snakeyaml-engine
branch
from
e8f01d4
to
54cf9e7
Compare
|
@jeffmay This is ready. Can you please merge this and release an RC version ( |
sideeffffect
force-pushed
the
snakeyaml-engine
branch
from
54cf9e7
to
28f4ba7
Compare
|
|
||
| def convertScalarNode(node: ScalarNode) = Either | ||
| .catchNonFatal(node.getTag match { | ||
| case Tag.INT if node.getValue.startsWith("0x") || node.getValue.contains("_") => |
There was a problem hiding this comment.
As the snakeyaml-engine readme says, "Only JSON Schema is supported." - that is, it doesn't support any literal formats beyond what's already in JSON. You can inspect the snakeyaml-engine impl of construct here and see that it's not doing anything fancier than e.g. String.toInt. So we can remove this entire case Tag.INT if node.getValue ... => block - it's not doing anything other than wasting CPU cycles and confusing / misleading future maintainers.
https://bitbucket.org/snakeyaml/snakeyaml-engine/src/master/#markdown-header-snakeyaml-engine-features
There was a problem hiding this comment.
Do you think this code would be useful once we add Core Schema support to snakeyaml-engine?
https://bitbucket.org/snakeyaml/snakeyaml-engine/issues/38/cannot-parse-integers-encoded-in
| throw new NumberFormatException(s"Invalid numeric string ${node.getValue}") | ||
| } | ||
| case Tag.BOOL => | ||
| Json.fromBoolean(flattener.construct(node) match { |
There was a problem hiding this comment.
See above on how snakeyaml-engine "only supports JSON schema". We have no need for callingconstruct method on literals, as snakeyaml-engine doesn't support any more literal formats than JSON itself already does.
# Conflicts: # build.sbt
# Conflicts: # circe-yaml/src/main/scala/io/circe/yaml/Parser.scala
|
I'll go ahead and merge this before any more merge conflicts appear. We can keep polishing this after it is in master |
closes #300
closes #301
closes #302
/cc @oyvindberg