Skip to content

Give Commands Options#41

Merged
ikesyo merged 10 commits intomasterfrom
give-commands-options
Dec 14, 2015
Merged

Give Commands Options#41
ikesyo merged 10 commits intomasterfrom
give-commands-options

Conversation

@mdiep
Copy link
Copy Markdown
Member

@mdiep mdiep commented Dec 11, 2015

This introduces some breaking changes to try to simplify the interaction of CommandType and OptionsType:

  • Add a typealias Options: OptionsType to CommandType
  • Make ClientErrors conform to ErrorType
  • Parse options from OptionsTypes automatically
  • Change CommandType.run() to take Options instead of CommandMode
  • Don't make AnyCommand conform to CommandType
  • Add AnyCommand.usage()

This would greatly simplify something like #40 because options are no longer parsed by commands. It also removes a decent amount of boilerplate from commands.

- Make `ClientError`s conform to `ErrorType`
- Parse options from `OptionsType`s automatically
- Change `CommandType.run()` to take `Options`
- Add `AnyCommand.usage()`
@mdiep
Copy link
Copy Markdown
Member Author

mdiep commented Dec 11, 2015

You can see how these changes affect Carthage in Carthage/Carthage#987.

@ikesyo
Copy link
Copy Markdown
Member

ikesyo commented Dec 12, 2015

I've tried the similar way and I couldn't reach to the goal at that time. 👏

I really like the idea, but I understand the @NachoSoto's concern that AnyCommand which does not conforms to CommandType seems confusing or misleading. Here are other suggestions:

  • CommandWrapper
  • CommandRunner
  • CommandExecutor with public let execute
  • CommandInvoker with public let invoke

Since CommandType has func run, CommandRunner might be a good one.

Comment thread Commandant/Command.swift
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense! 💯

@mdiep
Copy link
Copy Markdown
Member Author

mdiep commented Dec 13, 2015

I renamed AnyCommand to CommandWrapper, per @ikesyo's suggestion.

@NachoSoto
Copy link
Copy Markdown
Contributor

👍

Comment thread CommandantTests/OptionSpec.swift Outdated
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.

I think we should eventually move this to Result so that RAC and this can share it /cc @robrix

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like that idea

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.

@ikesyo
Copy link
Copy Markdown
Member

ikesyo commented Dec 14, 2015

Looks good! ✨

I added some minor fixes and I'm gonna merge this. 💣

ikesyo added a commit that referenced this pull request Dec 14, 2015
@ikesyo ikesyo merged commit 529a814 into master Dec 14, 2015
@ikesyo ikesyo deleted the give-commands-options branch December 14, 2015 08:21
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.

4 participants