Add validator to method_options with type :string, :numeric, :array and :hash

[maxmeyer]

Is

You can control what values of arguments/options thor accepts via :string, :boolean (options only), :numeric, :array and :hash. Sometimes that's not enough.

Given you've got an application like middleman I used regularly - a static site generator similar to jekyll - which has a server-command. This command has at least two options "relevant" for this use case:

• host
• port

A host name is defined in RFC 952 and is not allowed to include blanks or space characters -- look for "No blank or space characters are permitted as part of a name". It needs to match ^(?=.{1,255}$)[0-9A-Za-z](?:(?:[0-9A-Za-z]|-){0,61}[0-9A-Za-z])?(?:\.[0-9A-Za-z](?:(?:[0-9A-Za-z]|-){0,61}[0-9A-Za-z])?)*\.?$ to be valid. Using just :string is not enough to validate a host name.

A TCP port is defined as an integer from 0 to 65536 - see more in RFC6056. It needs to match (0..65535).include?(port). Just checking if it's a number is not enough either.

Today those checks are not possible, unfortunately.

Should

It should be possible to define a more granular check for :string, :numeric, :array and :hash. It should iterate over all values of an :array- and over all keys/values of a :hash-option/argument. The validator should have an easy to use API, e.g. validator#call -- if #call is used, you can use a proc/lambda-object as validator. To make it clear to users, that there are some more checks a validator must also have a description for those checks. Because a proc/lambda should be supported as well, this should be another option for #method_option.

Using the examples from above the validator, a validator should look like this:

Port Example

option :port, type: :numeric, validator: proc { |port| (0..65535).include?(port) }, validator_desc: 'Needs be in range 0-65535'

Hostname Example

option :hostname, type: :string, validator: proc { |name| /^(?=.{1,255}$)[0-9A-Za-z](?:(?:[0-9A-Za-z]|-){0,61}[0-9A-Za-z])?(?:\.[0-9A-Za-z](?:(?:[0-9A-Za-z]|-){0,61}[0-9A-Za-z])?)*\.?$/ === name }, validator_desc: 'Needs be valid hostname, see RFC 952 for more on this topic'

This PR implements this.

To be discussed

• Is it ok for you , that I add :validator + :validator_desc?
• Naming of new options, :validator and :validator_desc
  I used _desc because there's an option named :desc for a option-description already
• If a validator is given, having :validator_desc will be enforced, otherwise the developer may forget to add a clear description for end users, that a validator is in use and what the requirements of this validator are
• Add hint to help for validation. I thought that it might be helpful to make the user aware of a validation by adding the relevant information to the help command
• Is the interface of a validator ok for you, require it to have a #call-method? I thought that this is the most flexible and easy to use solution
• I added the information about the new options to the source_code-documentation. Is there any other place - besides the wiki - where I need to document the new options? I will update the wiki, if this PR is accepted.

Checklist From CONTRIBUTING

• It hasn’t been reviewed.
• It includes specs for new functionality.
• It includes documentation for new functionality.
• It changes relevant documentation, comments, or specs.
• It does not changes behavior of an existing public API, breaking backward compatibility. It only extends it
• It does not break the tests on a supported platform - waiting for CI to be finished.
• It merges cleanly (as of the time submitting).

[maxmeyer]

@sferik @wycats
Would you mind to review this PR? Thanks a lot.

[maxmeyer]

@sferik Is there any chance that you take a little bit of your time and review this? Thanks a lot in advance.

[maxmeyer]

@sferik Anything missing here?

[doudou]

👍

[rafaelfranca]

LGTM. I added a comment about missing tests and it needs a rebase

[rafaelfranca]

It is missing tests to arguments_spec like those one.

[maxmeyer]

What do you expect to be tested:

• Option valditaor also works if arguments are given?
• Adds validator to arguments as well?

[rafaelfranca]

You changed lib/thor/argument.rb to add validator support already. You add tests on this file to make sure when parsing the options the validators are respected, but there are no tests for when an argument is being parsed.

[tenebrousedge]

@rafaelfranca I'd love to see this code merged, and have some time available to work on it. I'm not understanding what kind of tests you would like to see.

[rafaelfranca]

Test for the argument parser that now also accepts validators.

[tenebrousedge]

@rafaelfranca The argument parser seems contradictory. It accepts an Array of Argument objects, then assigns those to a @switches variable, then later checks if that variable contains a Hash. The iteration within the file also seems very strange, and it's difficult to see how the "enum" argument functionality could be working. The argument code seems to need revision and clarification.

[benkoshy]

@rafaelfranca Is this the test that is missing? Perhaps I have misunderstood: benkoshy@b9a735d

(btw thank you for your open source contributions)