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

Replace broken open-coding of FromStr with derive(clap::ValueEnum) #197

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

MarijnS95
Copy link
Member

Fixes #194, closes #196, CC @SolidTux for testing.

We've seen that impl FromStr for Format missed the "exe" match arm in parsing (#194) despite being listed in the allowed values in our top-level documentation and help text.

Instead of fixing this mistake, remove open-coded FromStr entirely and rely on clap's excellent ValueEnum derive. This not only generates a complete parser (with the option for attributes to change variant names) but also automates generation of a list of possible values, getting rid of any ambiguity/duplication/copy-paste errors we might have or create in the future:

❯ cargo r -- build -h
...
Usage: x build [OPTIONS]

Options:
      ...
      --platform <PLATFORM>
          Build artifacts for target platform [possible values: android, ios, linux, macos, windows]
      --arch <ARCH>
          Build artifacts for target arch [possible values: arm64, x64]
      ...
      --format <FORMAT>
          Build artifacts with format [possible values: aab, apk, appbundle, appdir, appimage, dmg, exe, ipa, msix]
      --store <STORE>
          Build artifacts for target app store [possible values: apple, microsoft, play, sideload]

Finally, we also utilize its generated PossibleValue::get_name() to implement our open-coded Display implementations, ensuring any renames via clap attributes trickle through into how we print them.

We've seen that `impl FromStr for Format` missed the `"exe"` match arm
in parsing (#194) despite being listed in the allowed values in our
top-level documentation and help text.

Instead of fixing this mistake, remove open-coded `FromStr` entirely and
rely on `clap`'s excellent `ValueEnum` derive.  This not only generates
a **complete** parser (with the option for attributes to change variant
names) but also automates generation of a list of possible values,
getting rid of any ambiguity/duplication/copy-paste errors we might have
or create in the future:

    ❯ cargo r -- build -h
    ...
    Usage: x build [OPTIONS]

    Options:
          ...
          --platform <PLATFORM>
              Build artifacts for target platform [possible values: android, ios, linux, macos, windows]
          --arch <ARCH>
              Build artifacts for target arch [possible values: arm64, x64]
          ...
          --format <FORMAT>
              Build artifacts with format [possible values: aab, apk, appbundle, appdir, appimage, dmg, exe, ipa, msix]
          --store <STORE>
              Build artifacts for target app store [possible values: apple, microsoft, play, sideload]

Finally, we also utilize its generated `PossibleValue::get_name()` to
implement our open-coded `Display` implementations, ensuring any renames
via clap attributes trickle through into how we print them.
@MarijnS95 MarijnS95 merged commit f7c8252 into master Jan 3, 2025
34 checks passed
@MarijnS95 MarijnS95 deleted the mistakes branch January 3, 2025 14:53
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.

exe not recognized as a format
1 participant