-
-
Notifications
You must be signed in to change notification settings - Fork 142
Allow exposing CBOR simple values as VALUE_EMBEDDED_OBJECT #590
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
Merged
cowtowncoder
merged 8 commits into
FasterXML:2.x
from
iifawzi:587-simple-values-as-embedded-objects
May 11, 2025
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
f58c19c
Allow exposing cbor simple values as VALUE_EMBEDDED_OBJECT
iifawzi 69a3099
update README
iifawzi d62a70b
Merge branch '2.x' into 587-simple-values-as-embedded-objects
cowtowncoder f02755d
Renaming, add release notes
cowtowncoder ae2252d
Merge branch '2.x' into 587-simple-values-as-embedded-objects
cowtowncoder 2871cfe
clearing retained values for nextFieldName
iifawzi 704d7ec
minor reordering
cowtowncoder c9cbd52
minor tweaking, commenting
cowtowncoder File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few other places this must be done (everywhere where
_binaryValue = null;
is done)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought clearing it once at the beginning of nextToken could be enough, given it's used only once for major 7, and there is no partial assignment (always one byte).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't want to count on state not getting cleared for
nextFieldName()
variants, as they won't callnextToken()
.It may be that a combination of things could keep things consistent (as in, not exposing stale simple value) but when adding new features invariants might not hold.
But if you are confident after looking, that's fine, just LMK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the side; so far I've been exploring the async impl of CBOR; and I'm almost done with my draft, building almost all major types to explore the spec. In next weeks, I will start work on the actual organized PR, However, I'd like to know if you think there are higher priority issues that the community would like to be fixed. I'm enjoying contributing to the project in my free time, so aligning with priorities will make it even more valuable.
After the async parser, I believe looking at Avro issues is worth it, as I think it's the most used (just a guess from the number of issues)
Would appreciate your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, I got it, and well, I'm not 100% confident about the different combinations that might happen that can lead to exposing stale values, so yeah, better not to count on that. I will commit the change to clear it in places where binary values is cleared
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iifawzi On async impl: that sounds like something that would be well received so +1 for that. Avro does seem to get more attention that I'd have expected so that's a good thing to follow up on too. And maybe logical type support and whatever "new" (... I haven't been following up spec development) features for CBOR would be good as well.
(Avro, too, has logical type concept).
And ideally Proto3 would be supported by protobuf module. There's also question of replacing protoc-parser (reader for schema) that is deprecated with library that is to replace it (by same authors). I haven't had time to follow up on this, I think there's an issue filed to request it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the insights @cowtowncoder! I’ll keep those points in mind as I move forward after the async impl