Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Generate an error against unrecognized arguments #40

Merged
merged 1 commit into from
Dec 14, 2015

Conversation

ikesyo
Copy link
Member

@ikesyo ikesyo commented Nov 27, 2015

This is based on the implementations of #34 and #41. Resolves #2.

If a command specifies its associated options type in its public typealias Options = ... when that is OptionsType, the options will be evaluated and validated before the actual command runs.

/cc @norio-nomura, @mdiep

@norio-nomura
Copy link
Contributor

I confirmed that it works. 👍

@@ -86,6 +86,15 @@ public final class ArgumentParser {
}
}

private init(rawArguments: [RawArgument]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems weird to manually implement copy semantics. How about making this a struct instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we make ArgumentParser struct, option parsing using <| operator does not work. Consuming arguments is done in only in a operation and isn't propagated between each operations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another approach:

diff --git a/Commandant/ArgumentParser.swift b/Commandant/ArgumentParser.swift
index 6a63619..4ab5423 100644
--- a/Commandant/ArgumentParser.swift
+++ b/Commandant/ArgumentParser.swift
@@ -86,15 +86,6 @@ public final class ArgumentParser {
        }
    }

-   private init(rawArguments: [RawArgument]) {
-       self.rawArguments = rawArguments
-   }
-
-   /// Returns a new parser whose `rawArguments` is copied from the parser.
-   internal func copy() -> ArgumentParser {
-       return ArgumentParser(rawArguments: self.rawArguments)
-   }
-   
    /// Returns whether the given key was enabled or disabled, or nil if it
    /// was not given at all.
    ///
diff --git a/Commandant/Command.swift b/Commandant/Command.swift
index c23efb3..61fc0dd 100644
--- a/Commandant/Command.swift
+++ b/Commandant/Command.swift
@@ -27,16 +27,24 @@ public protocol CommandType {
 }

 /// A type-erased CommandType.
-public struct AnyCommand<ClientError>: CommandType {
+public struct AnyCommand<ClientError> {
    public let verb: String
    public let function: String
-   private let runClosure: CommandMode -> Result<(), CommandantError<ClientError>>
+   private let runClosure: InternalCommandMode -> Result<(), CommandantError<ClientError>>

    /// Creates a command that wraps another.
    public init<C: CommandType where C.ClientError == ClientError>(_ command: C) {
        verb = command.verb
        function = command.function
-       runClosure = { mode in command.run(mode) }
+       runClosure = { mode in
+           switch mode {
+           case let .Arguments(arguments):
+               return command.run(.Arguments(ArgumentParser(arguments)))
+
+           case .Usage:
+               return command.run(.Usage)
+           }
+       }
    }

    /// Creates a command that wraps another.
@@ -44,20 +52,22 @@ public struct AnyCommand<ClientError>: CommandType {
        verb = command.verb
        function = command.function
        runClosure = { mode in
-           if case let .Arguments(argumentsParser) = mode {
-               let parserCopy = argumentsParser.copy()
-               _ = C.Options.evaluate(.Arguments(parserCopy))
+           if case let .Arguments(arguments) = mode {
+               let parser = ArgumentParser(arguments)
+               _ = C.Options.evaluate(.Arguments(parser))

-               if let remainingArguments = parserCopy.remainingArguments {
+               if let remainingArguments = parser.remainingArguments {
                    return .Failure(unrecognizedArgumentsError(remainingArguments))
+               } else {
+                   return command.run(.Arguments(ArgumentParser(arguments)))
                }
            }

-           return command.run(mode)
+           return command.run(.Usage)
        }
    }

-   public func run(mode: CommandMode) -> Result<(), CommandantError<ClientError>> {
+   internal func run(mode: InternalCommandMode) -> Result<(), CommandantError<ClientError>> {
        return runClosure(mode)
    }
 }
@@ -72,6 +82,11 @@ public enum CommandMode {
    case Usage
 }

+internal enum InternalCommandMode {
+   case Arguments([String])
+   case Usage
+}
+
 /// Maintains the list of commands available to run.
 public final class CommandRegistry<ClientError> {
    private var commandsByVerb: [String: AnyCommand<ClientError>] = [:]
@@ -104,7 +119,7 @@ public final class CommandRegistry<ClientError> {
    ///
    /// Returns the results of the execution, or nil if no such command exists.
    public func runCommand(verb: String, arguments: [String]) -> Result<(), CommandantError<ClientError>>? {
-       return self[verb]?.run(.Arguments(ArgumentParser(arguments)))
+       return self[verb]?.run(.Arguments(arguments))
    }

    /// Returns the command matching the given verb, or nil if no such command

@mdiep mdiep mentioned this pull request Dec 11, 2015
@mdiep
Copy link
Member

mdiep commented Dec 11, 2015

I opened #41 as a basis for how I would make these changes.

This is based on the implementations of #34.
@ikesyo
Copy link
Member Author

ikesyo commented Dec 14, 2015

Rebased to master to reflect the changes in #41.

@mdiep
Copy link
Member

mdiep commented Dec 14, 2015

🎆

mdiep added a commit that referenced this pull request Dec 14, 2015
Generate an error against unrecognized arguments
@mdiep mdiep merged commit 0a6f59c into master Dec 14, 2015
@mdiep mdiep deleted the unrecognized-arguments branch December 14, 2015 13:55
@NachoSoto
Copy link
Contributor

So excited for this 💫

@norio-nomura
Copy link
Contributor

👏

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants