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

Problem with Must access entries in sequence #657

Merged
merged 2 commits into from
Nov 9, 2023
Merged

Problem with Must access entries in sequence #657

merged 2 commits into from
Nov 9, 2023

Conversation

dstepanov
Copy link
Contributor

@dstepanov dstepanov commented Nov 9, 2023

@yawkat Can you please take a look at this case? I think we should allow multiple calls to decodeKey after it returns null.

@yawkat
Copy link
Member

yawkat commented Nov 9, 2023

@dstepanov the reason i do not want to allow that is because Decoder does not require more than one decodeKey==null to succeed. In my mind, decodeKey advances the parser, and advancing the parser twice is not valid. I think it's a bug in the caller, like you can see in my fix it's possible to write deserializers in such a way that decodeKey is not called multiple times.

However all the other implementations of decodeKey handle multiple calls just fine. so i could be convinced to relax this requirement. but if we do it should be documented in Decoder.

Copy link

sonarqubecloud bot commented Nov 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

85.7% 85.7% Coverage
0.0% 0.0% Duplication

@dstepanov dstepanov merged commit 7b3863b into master Nov 9, 2023
11 checks passed
@dstepanov dstepanov deleted the ss branch November 9, 2023 12:27
@yawkat
Copy link
Member

yawkat commented Nov 9, 2023

fwiw if this becomes a common issue, i think we should relax the requirement.

@dstepanov dstepanov added the type: bug Something isn't working label Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants