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.

Sign up for GitHub

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

Jump to bottom

fix: Decoder does not represent the current node correctly #617

Merged
merged 3 commits into from
Nov 1, 2024
Merged

fix: Decoder does not represent the current node correctly #617

merged 3 commits into from
Nov 1, 2024

Conversation

Jojo4GH
Copy link
Contributor

@Jojo4GH Jojo4GH commented Sep 16, 2024

First bug from #607

Previously the node property on the decoders was not traversed in deep YAML trees (e.g. with YamlMapLikeInputBase and YamlListInput). This behaviour is different from the JSON serialization library and makes it impossible to implement a content based polymorphic KSerializer in a meaningful way without knowing of the whole YAML tree. See also the DecodingFromYamlNodeSerializer in YamlReadingTest.kt for an affected case

@Jojo4GH Jojo4GH mentioned this pull request Sep 16, 2024
Merged
2 tasks
charleskorn
charleskorn reviewed Sep 22, 2024
View reviewed changes
Copy link
Owner

@charleskorn charleskorn left a comment

Choose a reason for hiding this comment

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

Could you please add a test that demonstrates the issue this PR is trying to fix?

@Jojo4GH
Copy link
Contributor Author

Jojo4GH commented Sep 22, 2024

Could you please add a test that demonstrates the issue this PR is trying to fix?

There is the change in YamlReadingTest that gets fixed by this PR. Should I add more?

@charleskorn
Copy link
Owner

Could you please add a test that demonstrates the issue this PR is trying to fix?

There is the change in YamlReadingTest that gets fixed by this PR. Should I add more?

It's not clear to me how the two changes are related, so yes: please add a simple test case that shows the issue and how this fixes it.

@charleskorn charleskorn changed the title Fix Decoder not representing the current node correctly fix: Decoder not representing the current node correctly Oct 30, 2024
@Jojo4GH Jojo4GH requested a review from charleskorn November 1, 2024 01:43
@charleskorn
Copy link
Owner

Thanks @Jojo4GH!

@charleskorn charleskorn changed the title fix: Decoder not representing the current node correctly fix: Decoder does not represent the current node correctly Nov 1, 2024
@charleskorn charleskorn merged commit dbb29e7 into charleskorn:main Nov 1, 2024
7 checks passed
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 1, 2024

🎉 This PR is included in version 0.62.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charleskorn charleskorn Awaiting requested review from charleskorn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jojo4GH @charleskorn