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

Resolve 'bytes'/'binary' discrepancy #129

Merged
merged 4 commits into from
Nov 19, 2024
Merged

Conversation

popematt
Copy link
Contributor

Issue #, if available:

Follow up to #126

Description of changes:

  • Removed the use of bytes as a fragment keyword in favor of binary
  • Text fragments can now accept a mix of string literals a integers (which are interpreted as bytes)
  • I chose not to use bytes to be a general purpose input fragment because if we're combining fragments, we need to know what the intended format is to be. If bytes could represent binary or text, then something like (ivm 1 1) (bytes ...) would be ambiguous w.r.t. whether the IVM fragment should be converted to E0 01 01 EA or $ion_1_1.

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 toddjonker November 14, 2024 19:13
Comment on lines 6 to 11
branches:
# Branches from forks have the form 'user:branch-name' so we only run this job on pull_request events for
# branches that look like fork branches. Without this we would end up running this job twice for non-forked
# PRs, once for the push and then once for opening the PR.
# Taken from https://github.jparrowsec.cnmunity/t/how-to-trigger-an-action-on-push-or-pull-request-but-not-both/16662/10
- '**:**'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this no longer needed?

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 don't know if something like this is no longer needed. Merging this might cause the workflow to be run twice, but without this change, I wasn't getting it to run as part of the PR checks. Overall, though, this is a cheap/quick workflow, so I'm not too concerned.

@popematt
Copy link
Contributor Author

@toddjonker, I've got two approvals. I'm going to merge this, but if you have any concerns we can revisit this.

@popematt popematt merged commit 0b5d57a into amazon-ion:main Nov 19, 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.

3 participants