-
Notifications
You must be signed in to change notification settings - Fork 111
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
Allow custom writer configurations in IonSystemBuilder #781
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #781 +/- ##
============================================
+ Coverage 67.23% 67.31% +0.08%
- Complexity 5484 5498 +14
============================================
Files 159 159
Lines 23025 23044 +19
Branches 4126 4124 -2
============================================
+ Hits 15481 15513 +32
+ Misses 6262 6254 -8
+ Partials 1282 1277 -5 ☔ View full report in Codecov by Sentry. |
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 for your contribution!
Let me first say that this is a good change, and we will merge this feature. However, there are a few implementation details that seem surprising to me. I understand why you did it that way—it looks like they are equivalent to the existing code for setting the reader builder—so I'm going to ask one of the other Ion maintainers to provide a second opinion here.
We might decide to merge this as is, change some of the behaviors before merging, or change the writer builder and reader builder options in a separate PR.
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 for the contribution!
Co-authored-by: Zack Slayton <[email protected]>
Co-authored-by: Zack Slayton <[email protected]>
Co-authored-by: Zack Slayton <[email protected]>
I've discussed this with @zslayton and @tgregg, and we agreed that some of the existing behavior is unexpected, but we're not sure yet what the impact would be on consumers if we changed it. We're going to merge this PR without addressing any of the issues with the existing behavior, and I've created #783 for us to follow up on that. |
* Allow custom writer configurations in IonSystemBuilder * Update baseline.xml with JDK 17 * fix: fix typo in javadoc Co-authored-by: Zack Slayton <[email protected]> * fix: typo in javadoc Co-authored-by: Zack Slayton <[email protected]> * fix: typo in javadoc Co-authored-by: Zack Slayton <[email protected]> * Add default values on builders * Add javadoc * Add more tests on catalog * Fix tests on catalog --------- Co-authored-by: Zack Slayton <[email protected]> Co-authored-by: Matthew Pope <[email protected]>
Description of changes:
These changes allow the user to set their own configurations for the
IonTextWriterBuilder
andIonBinaryWriterBuilder
when using theIonSystemBuilder
, bringing more flexibility.Because this involved adding getters/setters, I had to update the SpotBugs
baseline.xml
file which was preventing the build from succeeding, and to fix a little bug in the Gradle configuration.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.