Adding dependencies to a CommandType's OptionType at init/evaluate time? about commandant HOT 5 CLOSED

By the way, while this does technically fulfill the requirements for DI, I'd rather be able to externally inject the dependencies. :/ Yeah, I do want to have my cake and eat it too.

from commandant.

I'll need to think about this, so it will take me a few days to respond.

from commandant.

My personal feelings is that Carthage command should be a thin translation layer between the command-line interface and the API. Because the translation layer is typed and largely handled by Commandant itself, it shouldn't require unit testing.

It's probably most useful to talk concretely about this with CheckoutCommand. Here's what I would do to simplify things and eliminate the need for testing:

1. Add an external function that's called from the run() method. This should take all the arguments. Something like checkout(useSSH: Bool, useSubmodules: Bool, useBinaries: Bool, projectDirectory: String, dependencies: [String]). (I'd like to do that anyway, so that all the Carthage commands are available as functions in CarthageKit.framework.)
2. Move all the logic from CheckoutCommand and its options struct into that function. Logic is what requires testing. At that point, CheckoutCommand is basically a mapping from the command-line representation onto the function. Data doesn't require testing.
3. The default value for project-directory could be changed to be nil, and then the checkout function could handle the default value. But I don't think that's necessary if CheckoutCommand is a thin mapping. And there are benefits to having the actual default there (e.g., try running carthage help checkout and look at the project-directory option.)

That's my feelings. I'm not sure how that will sit with you.

from commandant.

I agree, actually. (It also helps that I wrote this issue before I began my current refactor-spree in Carthage).

Ideally, I'd like to reach 100% test coverage. However, that's not possible for something as complex as Carthage. Even if it were, testing carthage.app isn't that high a priority for me. If we can refactor carthage.app to just be a simple translation from Commandant to CarthageKit, then I'd tolerate not testing carthage.app.

from commandant.

👍

Gonna close this issue then. Feel free to reopen if there's more to discuss!

from commandant.