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

Adds tests for annotate, flatten, and parse_ion system macros #138

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

popematt
Copy link
Contributor

Issue #, if available:

#88

Description of changes:

  • Test cases for annotate.
  • Test cases for flatten, which are almost identical to make_sexp except for the macro name, macro address, and that the output is an uncontained stream rather than a s-expression.
  • Test cases for parse_ion. I took a stance that passing the wrong type of argument to parse_ion in TDL is a (macro) compile time error rather than an expansion error. Feel free to discuss/disagree.
  • Minor fix in grammar.isl to correct the capitalization of the annot keyword for the denotes expectation.

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

conformance/system_macros/annotate.ion Outdated Show resolved Hide resolved
conformance/system_macros/annotate.ion Show resolved Hide resolved
conformance/system_macros/flatten.ion Outdated Show resolved Hide resolved
Comment on lines +96 to +98
(then "from values that look like system values"
(text '''(:parse_ion "$ion_1_1 $ion_literal::$ion_symbol_table::{symbols:[]}")''')
(produces $ion_symbol_table::{symbols:[]}))
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems dangerous because the expanded stream now has a system value. How do we prevent it from being interpreted as such on a subsequent roundtrip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it should still have $ion_literal on it, but the rules around $ion_literal still need to be cleared up. Does the $ion_literal annotation get silently dropped in the application reader before handing the value off the application/user, or does it stick around?

I don't think this problem is exclusive to parse_ion, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it has to stick around, because not having it allows $ion_symbol_table to be the first annotation, which changes the semantics of the stream. Happy to punt on that though.

@popematt popematt merged commit 056cb4e into amazon-ion:main Nov 27, 2024
1 check passed
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.

2 participants