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

Add new shouldUseExecutableName configuration property (Issue #295) #501

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

smvz
Copy link

@smvz smvz commented Oct 4, 2022

Description

This adds the shouldUseExecutableName property to CommandConfiguration, allowing the command name to be derived from the executable's file name.

The property defaults to false, both because subcommands using it is probably undesirable and to preserve existing behaviour after updating the package.

This also adds /DerivedData to the git ignore list.

Detailed Design

/// A Boolean value indicating whether to use the executable's file name for the command name.
public var shouldUseExecutableName: Bool

The CommandConfiguration initialisers use a default value of false, serving to allow existing code to compile and to avoid an unexpected change in behaviour. When the value is true, if both commandName and _superCommandName are nil, the command name will be derived from the first command line argument, i.e. the path to the running executable.

Change to associated code paths is minimal, specifically ParsableCommand._commandName has been expanded in a way that preserves the original outcome when the new property is false.

The thinking behind ignoring this value when either commandName or _superCommandName have been set is that if the command name has been explicitly set, this should be used. If the super command name has been set, this implies a relationship with another command where a dynamic command name may lead to inconsistent help texts.

Two new internal symbols - see diff for full API documentation.

/// Will generate a tool name from the name of the executed file if possible.
static var executableName: String {}

/// Returns a new single-quoted string if this string contains any characters
/// from the specified character set. Any existing occurrences of the `'`
/// character will be escaped.
func quotedIfContains(_ chars: CharacterSet) -> String {}

Documentation Plan

Includes API documentation for the new symbols and minorly updates docs to expose the new property.

Test Plan

Includes unit tests for the string quoting. I'm unaware of a guaranteed executable name when running tests and therefore this does not include tests for the command name generation.

Source Impact

Existing users should not be impacted by this change.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

This adds the `shouldUseExecutableName` property, allowing the
command name to be derived from the executable's file name.

The property defaults to false, both because subcommands using
it is probably undesirable and to preserve existing behaviour
after updating the package.
@smvz smvz changed the title Add new shouldUseExecutableName configuration property Add new shouldUseExecutableName configuration property (Issue #295) Oct 5, 2022
Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, @QuaqSim!

The interaction with commandName and _superCommandName is definitely tricky. I'm not 100% sure that ignoring the new property when commandName is non-nil is always the right call. In particular, I think during development that could lead to an unexpected display (since the executable there takes on the target name, which may not be the same as the intended name after installing). I think you're right that _superCommandName being set should override everything else.

If you were designing this configuration from scratch, without any worry about source compatibility, how would you design it?

@smvz
Copy link
Author

smvz commented Oct 10, 2022

I'm not 100% sure that ignoring the new property when commandName is non-nil is always the right call. In particular, I think during development that could lead to an unexpected display (since the executable there takes on the target name, which may not be the same as the intended name after installing).

If I'm interpreting this correctly, you're suggesting that if commandName is non-nil and shouldUseExecutableName is true, a developer may observe what she believes to be the executable name being used, which won't actually be used after installing.

For example, if the target name and commandName are both "some-command", this would appear to be using the executable name until it is renamed.

If we agree that shouldUseExecutableName true should never be used when commandName is non-nil, I could make that a fatalError(). Tree init(root:) and CommandParser init(_:) looks like a good place to do this, similar to how they currently handle recursive subcommands.

If you were designing this configuration from scratch, without any worry about source compatibility, how would you design it?

Without worrying about source compatibility, I would use a CommandName enumeration in order to clearly assign exactly one selection. Something like...

public enum CommandName {
  case snakeCasedTypeName
  case executableName
  case explicit(String)
}

public struct CommandConfiguration {
  public var commandName: CommandName
  ...
  
  public init(
    commandName: CommandName = .snakeCasedTypeName
    ...

I'm not sure how _superCommandName plays into that. Perhaps as an additional

indirect case niceDescriptiveNameImFailingToThinkOf((superCommandName: String, commandName: CommandName))

Only .snakeCasedTypeName and .explicit(String) would be valid values here.

This doesn't change test behaviour in any way. Single quotes were escaped
unnecessarily for some tests, these escapes have been removed to make
them consistent and clearer.
@smvz
Copy link
Author

smvz commented Sep 23, 2023

Do you have any feedback on how you would like me to proceed with this? I think the fatalError() suggestion would cover your concern with ignoring the new property in a reasonable way, but I'm unsure whether that's the approach you want to take there.

Foundation imports were reduced to only the necessary protocols or
types in the main branch. After merging, we need to import
`Foundation.URL` for use in `executableName`.
@dcantah
Copy link
Member

dcantah commented May 31, 2024

Oop, I shoulda looked at the open PRs before hacking on #641 😅. I went with making it the default and foregoing a new config option, but this seems more sane from a back compat standpoint.

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