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

Add basic conformance tests for IVMs, system symbols, and local symtabs. #96

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

toddjonker
Copy link
Contributor

Description of changes:

I believe the only 1.1-specific impact here is the test asserting that $10 is $ion_literal.

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

Comment on lines +6 to +7
(toplevel '#$ion_1_0' null '#$ion_1_1' null)
(produces null null))
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the nulls necessary? I.e. is it possible to have (produces /* nothing */)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible to do that, but having some stub data there ensures that the IVM is the only thing skipped.



// Basic symbol expansion
(ion_1_0 (toplevel $ion_symbol_table::{symbols:["a"]})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion—we should make sure that (toplevel $3::{ $7: ["a"] } ) here would also produce the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, will do.


(ion_1_1
(toplevel '#$10')
(produces $ion_literal))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't recall seeing this in the spec. Can we put a "todo" here until the system module is defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/amazon-ion/ion-docs/blob/ion-11-specification/src/whatsnew.adoc#system-symbol-table-changes

Though I got the address here wrong, I'll fix that.

I'd be shocked if there's any debate whether $ion_literal and $ion_encoding are included, do you think there will be debate over their specific addresses?

Copy link
Contributor

@popematt popematt Jun 4, 2024

Choose a reason for hiding this comment

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

I see that now.

Can you remind me what $ion_literal is for?

EDIT—I just did a search in the tip of the ion-11-specification branch of ion-docs, and $ion_literal is not mentioned anywhere other than in the System Symbol Table section of the What's New chapter.

Copy link
Contributor Author

@toddjonker toddjonker Jun 4, 2024

Choose a reason for hiding this comment

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

$ion_literal is how one treats as application data something looking like an encoding directive.

At top-level $ion_literal::$ion_encoding::() expands to $ion_encoding::() rather than being evaluated as a directive. This avoids holes in the data model by ensuring anything can be written at top level.

It's a gap in the spec, and I know some edge cases we need to iron out, so cut issue amazon-ion/ion-docs#322

@toddjonker toddjonker merged commit 9376c98 into amazon-ion:master Jun 4, 2024
@toddjonker toddjonker deleted the basics branch June 4, 2024 21:37
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