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

Get Review from Amazon Ion Team & Other Stakeholders #1

Open
hohle opened this issue Jun 21, 2023 · 9 comments
Open

Get Review from Amazon Ion Team & Other Stakeholders #1

hohle opened this issue Jun 21, 2023 · 9 comments
Assignees

Comments

@hohle
Copy link
Owner

hohle commented Jun 21, 2023

https://gist.github.com/hohle/1203846da7579950be4b70b93d7cee19

@hohle hohle self-assigned this Jun 21, 2023
@hohle
Copy link
Owner Author

hohle commented Jun 21, 2023

Comments previously tracked in amazon-ion/ion-java#82

@jobarr-amzn
Copy link
Contributor

Digging up my old notes from an incomplete review of this RFC:


Questions about RFC rendering:

  • should we do anything about the awkward word breaks in commented hexadecimal examples?

    • Is this a use case for .nf?
  • Is this desired?

 o Every top-level value (and all the hierarchical data within it) is
      interpreted with respect to the current symbol table at the point
      where the value starts.

I'm not sure what the "is this desired?" part means anymore, I think that might be referring to the rendering of the following input as above:

o Every top-level value (and all the hierarchical data within it) is
  interpreted with respect to the current symbol table at the point
  where the value starts.

Is it expected that lines after the first be indented more than the first line?

@jobarr-amzn
Copy link
Contributor

I've gone through about 60 pages of this, I had not yet reviewed Appendix A (the ANTLR grammar).

I have a fork where I was updating the RFC text with changes/clarifications to the spec: https://github.com/jobarr-amzn/ion-rfc

I see that you also have been updating this since I forked it, but some changes (e.g. at least using hex representations like 0xD for type codes in the binary spec descriptions) aren't in the RFC here.

See: main...jobarr-amzn:ion-rfc:main .

I can rebase and fire a PR out.

@jobarr-amzn
Copy link
Contributor

We are not confident that the ANTLR grammar specifies Ion correctly, see e.g. amazon-ion/ion-docs#179 .

So perhaps the grammar there needs additional disclaimer?

@hohle
Copy link
Owner Author

hohle commented Jun 28, 2023

Is it expected that lines after the first be indented more than the first line?

No, it's not expected. I'm not sure where I got the list convention I was using, but it seems like there are better macros for lists. I'll post an updated version tomorrow.

@hohle
Copy link
Owner Author

hohle commented Jun 28, 2023

main...jobarr-amzn:ion-rfc:main

I've merged these changes in as of this morning with some minor changes that seem to have been made to the spec since they were included (e.g. only UTF-8 is valid for text encoding now?).

@hohle
Copy link
Owner Author

hohle commented Jun 28, 2023

Going back through different RFC authoring docs it seems there is now significantly more support for XML based sources. I may spend some time translating everything over which is probably a better long-term approach than reformatting lists and other items.

If there are other changes to merge, I can hold off until those are ready but will start working on a PoC and comparing output based on the currently generated output.

@jobarr-amzn
Copy link
Contributor

For what it's worth the Ion 1.1 specification is being finalized on GitHub right now, in AsciiDoc.

PartiQL also uses AsciiDoc, and we're moving in that direction. I see that there is some tooling (e.g. metanorma/metanorma-ietf) for working with AsciiDoc and generating RFC-formatted output, but I'm not sure how distant our documentation is from feasibly working with whatever input constraints that tooling has.

@hohle
Copy link
Owner Author

hohle commented Jun 29, 2023

Thanks, I'll look into it.

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

No branches or pull requests

2 participants