Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support tagged nodes #7

Merged
merged 5 commits into from
Sep 21, 2019

Conversation

EdwarDDay
Copy link
Contributor

- implement charleskorn#4
- support yaml tags for polymorphic de-/serialization
@charleskorn
Copy link
Owner

Hey @EdwarDDay, thanks for the PR! Before I take a closer look, could you please fix the compilation error Travis is reporting?

- fix java 8 bug
@codecov-io
Copy link

codecov-io commented Sep 10, 2019

Codecov Report

Merging #7 into master will increase coverage by 0.14%.
The diff coverage is 92.15%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master      #7      +/-   ##
===========================================
+ Coverage     93.55%   93.7%   +0.14%     
- Complexity      101     108       +7     
===========================================
  Files             9       9              
  Lines           388     429      +41     
  Branches         76      85       +9     
===========================================
+ Hits            363     402      +39     
- Misses           10      13       +3     
+ Partials         15      14       -1
Impacted Files Coverage Δ Complexity Δ
src/main/kotlin/com/charleskorn/kaml/YamlNode.kt 100% <100%> (ø) 1 <0> (ø) ⬇️
...main/kotlin/com/charleskorn/kaml/YamlNodeReader.kt 88.57% <100%> (+0.16%) 42 <1> (+1) ⬆️
src/main/kotlin/com/charleskorn/kaml/Yaml.kt 87.5% <100%> (ø) 5 <0> (ø) ⬇️
src/main/kotlin/com/charleskorn/kaml/YamlInput.kt 92.3% <86.95%> (-0.34%) 1 <0> (ø)
src/main/kotlin/com/charleskorn/kaml/YamlOutput.kt 95.55% <93.75%> (+1.8%) 30 <10> (+6) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65c9f1c...d4e52a1. Read the comment docs.

- fix java 8 bug
src/main/kotlin/com/charleskorn/kaml/YamlOutput.kt Outdated Show resolved Hide resolved
src/main/kotlin/com/charleskorn/kaml/YamlOutput.kt Outdated Show resolved Hide resolved
src/test/kotlin/com/charleskorn/kaml/YamlReadingTest.kt Outdated Show resolved Hide resolved

context("given some input representing an object with a tagged value as a key") {
val input = """
!<sealedInt> { }: something
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the intent of this test clear, could you remove the { } and replace it with a string (eg. !<sealedInt> thing: something)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not yet possible, but I can create another PR after that one. Currently scalar nodes with tags are not supported and supporting them would be a new feature with a new PR in my opinion.

src/test/kotlin/com/charleskorn/kaml/YamlTaggedTest.kt Outdated Show resolved Hide resolved

context("given some tagged input representing an object where the resulting type should be a sealed class (int)") {
val input = """
element: !<sealedInt>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to support !sealedInt instead of !<sealedInt>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that can be done via the %TAG directive, but I'm not sure how to do that.

src/test/kotlin/com/charleskorn/kaml/YamlWritingTest.kt Outdated Show resolved Hide resolved
- cleanup tests
@charleskorn
Copy link
Owner

Hey @EdwarDDay, thanks for incorporating my feedback and apologies for the delay in getting back to you. This is looking really good. The only things I'd like to see before I merge this are:

  • More tests for reading tagged input nodes - for example, the logic in YamlTaggedInput.getCurrentLocation() is never exercised (take a look at the tests for other kinds of nodes for how this is used to generate exceptions with location information) and none of the scalar decoding methods (eg. decodeInt()) are exercised. I'm not suggesting to test these scalar decoding methods individually, but it does seem that they're not being used by the current tests which suggests they might not be necessary and can be removed.

  • If the scalar decoding methods are required, I'd like to see them simplified a bit to remove the duplication and risk of things getting out of sync - for example, I'm imagining decodeInt() could look something like:

    override fun decodeInt(): Int = decodeScalar { decodeInt() }

    where decodeScalar is a private inline function that does the check for the current index and picks the appropriate decoder

- remove unused code
- ignore tags on lists an maps if not needed
@EdwarDDay
Copy link
Contributor Author

I removed the decode[Int|Short|Unit|...] methods, because these are not yet used. The getCurrentLocation() is now tested. Additionally a tag is ignored, if it is not needed.

@charleskorn
Copy link
Owner

Looks good to me, thanks @EdwarDDay!

@charleskorn charleskorn merged commit 164fef4 into charleskorn:master Sep 21, 2019
@EdwarDDay EdwarDDay deleted the feature/tagged-node-support branch September 22, 2019 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants