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

Removes assertions in IonReaderSystemTextX.stringValue(). #703

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

tgregg
Copy link
Contributor

@tgregg tgregg commented Jan 24, 2024

Issue #, if available:
Closes #702

Description of changes:
The assertion on line 609 was unnecessary; the assertion on line 611 was both wrong (should have been >=) and unnecessary because the next line throws an error that is more helpful than the assertion.

Before this change, the added test failed with AssertionError, which was wrong.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

codecov bot commented Jan 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dd2fdbf) 67.25% compared to head (df790b7) 67.26%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #703      +/-   ##
============================================
+ Coverage     67.25%   67.26%   +0.01%     
- Complexity     5486     5488       +2     
============================================
  Files           159      159              
  Lines         23027    23025       -2     
  Branches       4128     4126       -2     
============================================
+ Hits          15486    15487       +1     
  Misses         6258     6258              
+ Partials       1283     1280       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@popematt popematt left a comment

Choose a reason for hiding this comment

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

Should we try to eliminate other asserts as well? (Not in this PR.)

@tgregg
Copy link
Contributor Author

tgregg commented Jan 25, 2024

Should we try to eliminate other asserts as well? (Not in this PR.)

Generally, yes. There are a lot of usages. The ones I spot checked at least look like they're being used properly and won't surface in error conditions that can be triggered by users.

@tgregg tgregg merged commit 581f868 into master Jan 25, 2024
21 of 35 checks passed
@tgregg tgregg deleted the remove-text-stringvalue-assertions branch January 25, 2024 00:29
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.

Remove assertions in IonReaderSystemTextX.stringValue()
2 participants