Skip to content

Adds experimental --ion-version option#128

Merged
popematt merged 9 commits intoamazon-ion:masterfrom
popematt:ion-version-flag
Jul 2, 2024
Merged

Adds experimental --ion-version option#128
popematt merged 9 commits intoamazon-ion:masterfrom
popematt:ion-version-flag

Conversation

@popematt
Copy link
Copy Markdown
Contributor

@popematt popematt commented Jul 2, 2024

Issue #, if available:

#85

Description of changes:

Changes are mostly broken up by commits

  • Removes the beta namespace
  • Removes the passthrough dump command in favor of using clap's alias feature to achieve the same effect.
  • Adds --unstable/-X to opt into experimental features.
  • Adds an is_porcelain() function to the IonCliCommand trait. If implemented to return true, it gets a warning at the bottom of the command help that the output is not intended to be suitable for machine-readable use cases.
  • Adds --ion-version option

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 jobarr-amzn and zslayton July 2, 2024 00:24
Comment thread README.md
non-goal.
help Print this message or the help of the given subcommand(s)
cat Prints all Ion input files to the specified output in the requested format.
count (UNSTABLE) Prints the number of top-level values found in the input stream.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What makes count unstable, anyway?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have no idea, but it was in the beta namespace, so I'm leaving it as unstable for now. We can fix that in a separate PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, it is possible that #104 could eventually replace count, so maybe that's a good reason to leave it a "unstable" for now.

fn configure_args(&self, command: Command) -> Command {
command.with_input().with_output().with_format()
command
.alias("dump")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

Comment on lines +28 to +43
/// Indicates whether this command is stable (as opposed to unstable or experimental).
/// Namespaces should almost always be stable.
fn is_stable(&self) -> bool {
true
}

/// Whether the output format is machine-readable.
///
/// Commands that are "plumbing" should default to putting one output (result, value, document)
/// on each line in a machine-readable format (file name, Ion value(s), integers, booleans)
/// without any prose or table formatting, etc.
///
/// See https://git-scm.com/book/en/v2/Git-Internals-Plumbing-and-Porcelain#_plumbing_porcelain
fn is_porcelain(&self) -> bool {
false
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the defaults be flipped here? I'd rather default to assuming that commands are unstable and porcelain, and have commands intentionally vouch that they are stable or plumbing.

Copy link
Copy Markdown
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 mind flipping them.

Copy link
Copy Markdown
Contributor

@jobarr-amzn jobarr-amzn left a comment

Choose a reason for hiding this comment

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

I'm happy to approve it as-is, I'd like to change the default unstable/porcelain values but there's no need to couple that here.

@popematt popematt merged commit c47232b into amazon-ion:master Jul 2, 2024
@popematt popematt deleted the ion-version-flag branch July 2, 2024 22:38
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