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

💥 Command Aliases #155

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

💥 Command Aliases #155

wants to merge 1 commit into from

Conversation

phatblat
Copy link
Member

@phatblat phatblat commented Aug 24, 2020

Implements #153.

Opening this PR as a draft for discussion. I've got a simple/dumb implementation of aliases working in mas and you can see the behavior below. The install command was given an alias of i, but the help output lists the install command twice.

Available commands:

   account     Prints the primary account Apple ID
   help        Display general or command-specific help
   home        Opens MAS Preview app page in a browser
   info        Display app information from the Mac App Store
   install     Install from the Mac App Store
   install     Install from the Mac App Store
   ...

↪ sudo mas uninstall 803453959
Password:
==> App moved to trash: /private/var/root/.Trash/Slack 20-31-11-388.app

↪ mas i 803453959
==> Downloading Slack
#####################################################------- 88.0% Installing

This was achieved by:

  1. Adding CommandProtocol.aliases
  2. Registering commands again, once for each alias name

Modifying the CommandProtocol is a breaking change and each user of this library would need to add the alias property to every command, even if the command has no aliases. The 2nd item causes the command duplication but could be improved by extending the idea of an alias into the registry and auto-generating the help description with something like:

Available commands:

   install     Install from the Mac App Store
   i     Alias for install

Perhaps a better approach is to register aliases differently instead of modifying the model. Going to think some more about this but welcome any feedback or suggestions.

@phatblat phatblat requested review from a team August 24, 2020 02:52
@phatblat phatblat mentioned this pull request Aug 24, 2020
@phatblat
Copy link
Member Author

Rebased onto master so compare view is much cleaner now.

@muescha
Copy link

muescha commented Jun 18, 2022

and other solution would be a Wrapper for a command -

AliasCommandWrapper / Alias:

public struct Alias<ClientError: Error>: CommandProtocol {
	// [...]
	fileprivate init<C: CommandProtocol>(_ command: C, alias: String) where C.ClientError == ClientError {
		verb = alias
		function = "alias for '${command.verb}': {$command.function}"
		run = command.run
		usage = command.usage 
	}
}

usage:

registry.register(Alias(ListCommand(), alias: "L"))

drawback: the alias strings are not defined at the command class

@muescha
Copy link

muescha commented Jun 18, 2022

Modifying the CommandProtocol is a breaking change and each user of this library would need to add the alias property to every command, even if the command has no aliases.

is it not possible to make the alias as optional, so that it not need to add and it is not a breaking change?

Copy link

@muescha muescha left a comment

Choose a reason for hiding this comment

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

changes to display alias in commands list

@@ -96,6 +99,10 @@ public final class CommandRegistry<ClientError: Error> {
{
for command in commands {
commandsByVerb[command.verb] = CommandWrapper(command)
// Register command for each additional alias
for alias in command.aliases {
commandsByVerb[alias] = CommandWrapper(command)
Copy link

Choose a reason for hiding this comment

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

Suggested change
commandsByVerb[alias] = CommandWrapper(command)
let wrappedCommand = CommandWrapper(command)
wrappedCommand.verb = alias
wrappedCommand.function = "alias for '${command.verb}': {$command.function}"
commandsByVerb[alias] = wrappedCommand

@@ -20,6 +20,9 @@ public protocol CommandProtocol {
/// `help`).
var verb: String { get }

/// Optional list of additional verbs which can be used to invoke this command.
var aliases: [String] { get }
Copy link

@muescha muescha Jun 18, 2022

Choose a reason for hiding this comment

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

Suggested change
var aliases: [String] { get }
var aliases: [String]? { get }

but let aliases be optional and not defined (null)

PS:
need to add

extension CommandProtocol {
    var aliases: [String]? { return nil }
}

or do just a default implementation:

extension CommandProtocol {
    var aliases: [String] { return [] }
}
``

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