Add type casting via :cast option

[katafrakt]

This is an implementation of the idea from comments in #146 to provide a Dry Types support via a :cast option. It leverages the fact that Dry Types types are callable and adds for free an ability to use any callable as a caster.

Usage with Dry Types

1. Add dry-types to the Gemfile
2. Define the app with Dry Types

Types = Dry.Types()

module WithDryTypes
  extend Dry::CLI::Registry

  class Info < Dry::CLI::Command
    desc "Display info"

    argument :type, desc: "Type of the information", required: true
    argument :lines, desc: "Number of lines to show", cast: Types::Coercible::Integer, default: 10
    option :sudo_mode, desc: "Enable sudo mode?", cast: Types::Params::Bool.default(false)
    option :uid, cast: ->(uid) { uid.to_i }

    def call(**options)
      puts JSON.dump(options)
    end
  end

  register "info", Info
end

Dry.CLI(WithDryTypes).call

4. Test it

❯ spec/support/fixtures/with_dry_types info system 14 --sudo_mode true
{"lines":14,"sudo_mode":true,"type":"system","args":["14"]}

[noraj]

Looks perfect to me

[timriley]

This looks great, thank you @katafrakt! A really nice enhancement, and leveraging Dry Types via the extension is a nice way to reinforce our ecosystem.

Since we're adding the type: option and this #type_cast method, I wonder if there's a way we might be able allow for lightweight type casting for users who don't want to bring in Dry Types? For example, could we have it check for a proc even if the dry_types extension isn't loaded, and that any exception raised would be considered a type failure? (Alternatively, we could provide our own Dry::CLI::TypeError exception that we could tell users to raise explicitly).

Either way, I'm happy for this part to go in, and perhaps we could consider the idea above via another PR (if we did decide to go ahead).

Can you please add a CHANGELOG note before merging?

[timriley]

For the built-in alternative, I am suggesting something slightly different to #146 — I don't think we need to provide our own built-in set of types, and instead require the user to do what they need (or just pull in dry-types and get a rich types library out of the box).

[katafrakt]

@timriley I did not add :type option in this PR. It was already there and it can take :boolean, :flag or :array.

... and TBH immediately after having written the above, I started to think that it's probably not right to have an option that can either take some symbols, or a Dry Types type, or a proc in the future. It just does not feel like a great API design, especially given that current symbol "types" modify how the parsing of the CLI arguments work.

So, pushing back on my own proposal, perhaps we could rather introduce a new option. I'm thinking of :cast to be very clear about the intention. So we would have

argument :lines, desc: "Number of lines to show", cast: Types::Coercible::Integer, default: 10

I think this way it could be mixed with :array type and still make sense.

What do you think?

[timriley]

@katafrakt haha, thank you for setting the record straight, I had clearly forgotten about that feature when I shared my comment!

Yes, I agree with your instinct here, and I'm happy with the name cast:, let's do it. 👍🏼

[fnordfish]

(Sorry for sliding in like this - it just so happens that I just faced a similar question on a different project)

It might be worth noting that there might be conflicting "default" values, and which wins:

# 5 or 10?
argument :lines, desc: "Number of lines to show", cast: Types::Coercible::Integer.default(5), default: 10

# does this even work?
argument :lines, desc: "Number of lines to show", cast: Types::Coercible::Integer.default(10)

Also, what would happen when the arguments default value conflicts with the cast type:

argument :lines, desc: "Number of lines to show", cast: Types::Coercible::Integer, default: "ten"

[katafrakt]

Thanks for the comment @fnordfish, this is actually really good and important question.

My gut feeling is that casting should happen last, after applying the default. So your first example will result 10, second will work and result in 10, the last one would cause casting exception. I believe this leads to least surprises, all things considered. But I'm open to other perspectives.

Of course, whatever the decision is, it should be clearly documented.

[fnordfish]

@katafrakt Thanks. It's the behavior I would expect, so no argument there :)
In my project, I disallowed setting a "argument default", and require the Type to define the default value. Not sure if that makes sense here, but it certainly removes ambiguity.

[timriley]

My gut feeling is that casting should happen last, after applying the default.

Agreed!

[timriley]

Note that we have a similar scenario in Dry Configurable, where settings can have both a default: as well as a constructor: (which is what we're calling cast: here).

[katafrakt]

@noraj @timriley @fnordfish I pushed revamped version. Not much left from the previous one. PR description updated.