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

Write support for Ion 1.1 system symbols #941

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

popematt
Copy link
Contributor

Issue #, if available:

None

Description of changes:

  • Updates the FlexSym related methods in WriteBuffer to be able to write system symbols.
  • Adds APIs that accept SystemSymbol to IonRawWriter_1_1.
  • Adds support for writing Ion 1.1 system symbols for both the text and binary writers.
  • Updates IonManagedWriter_1_1 to use system symbols for writing encoding directives and to move all of the system symbols out of user-symbol space.
  • (Temporarily) disabled tests that use the Ion 1.1 reader and writer for a round trip of any sort because the reader does not have full support for system symbols yet.

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

@popematt popematt requested review from tgregg and zslayton September 20, 2024 18:30
* Writes one annotation for the next value.
* [writeAnnotations] may be called more than once to build up a list of annotations.
*/
fun writeAnnotations(annotation0: SystemSymbols_1_1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: reading this I was a bit thrown off by writeAnnotations being plural, annotation0 being singular, and SystemSymbols_1_1 being plural. Could this flavor of the method have a singular name?

writer.writeAnnotation(SystemSymbols_1_1.ION_ENCODING)

I see it's part of an interface--if changing it comes with a lot of baggage then just leave it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally made it plural because it overloads the existing writeAnnotations methods which are intended to provide a pseudo-varags API without the overhead of varargs.

    fun writeAnnotations(annotation0: Int)
    fun writeAnnotations(annotation0: Int, annotation1: Int)
    fun writeAnnotations(annotations: IntArray)
    fun writeAnnotations(annotation0: CharSequence)
    fun writeAnnotations(annotation0: CharSequence, annotation1: CharSequence)
    fun writeAnnotations(annotations: Array<CharSequence>)

I'm fine with changing it, but I'd prefer deal with the naming later—perhaps as part of a larger-scale review/revisiting of new API names/signatures before merging the Ion 1.1 branch into the main branch.

@popematt popematt merged commit 4b373e5 into amazon-ion:ion-11-encoding Sep 20, 2024
21 of 34 checks passed
@popematt popematt deleted the system-symbols-b branch September 24, 2024 06:09
tgregg pushed a commit that referenced this pull request Dec 13, 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.

3 participants