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

Change IonSystemBuilder's default charset to UTF-8 #802

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

popematt
Copy link
Contributor

@popematt popematt commented Apr 22, 2024

Issue #, if available:

Fix #783

Description of changes:

  • I decided to change the default charset for IonSystemBuilder to UTF-8
  • I looked into allowing the different reader/writer builders to use different catalogs, but it turns out that the IonSystem does interact with the catalog outside of the reader/writer builders through createSharedSymbolTable(). I don't think there's a good way to define how that method should interact with multiple catalogs since a shared symbol table could be present in one catalog but not in another.
  • The changes in IonSystemLite are all non-functional. I cleaned up some formatting near the reader/wwriter builder member fields (and updated the copyright header).

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

@popematt popematt requested a review from tgregg April 22, 2024 21:33
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.41%. Comparing base (3c1b6b1) to head (bdf84c6).
Report is 34 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #802      +/-   ##
============================================
+ Coverage     67.23%   67.41%   +0.17%     
- Complexity     5484     5518      +34     
============================================
  Files           159      160       +1     
  Lines         23025    23077      +52     
  Branches       4126     4126              
============================================
+ Hits          15481    15557      +76     
+ Misses         6262     6246      -16     
+ Partials       1282     1274       -8     

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

Copy link
Contributor

@tgregg tgregg left a comment

Choose a reason for hiding this comment

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

Will approve as long as a dry run looks good

@popematt popematt merged commit 5158706 into amazon-ion:master Apr 23, 2024
18 of 35 checks passed
@popematt popematt deleted the writerbuilder branch April 23, 2024 17:45
tgregg added a commit that referenced this pull request Apr 29, 2024
tgregg added a commit that referenced this pull request Apr 30, 2024
tgregg added a commit that referenced this pull request Apr 30, 2024
linlin-s pushed a commit that referenced this pull request May 9, 2024
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.

Clean up unexpected interactions between IonSystemBuilder and the reader and writer builders
2 participants