Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 88 additions & 1 deletion lib/elixir/lib/option_parser.ex
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ defmodule OptionParser do
@type errors :: [{String.t, String.t | nil}]
@type options :: [switches: Keyword.t, strict: Keyword.t, aliases: Keyword.t]

defmodule InvalidOptionError do
Copy link
Member

Choose a reason for hiding this comment

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

We're calling this InvalidOptionError but we're raising it with multiple errors for different options. Maybe we can call it InvalidOptionsError?

defexception [:message]
end

@doc """
Parses `argv` into a keywords list.

Expand Down Expand Up @@ -64,6 +68,9 @@ defmodule OptionParser do

If a switch can't be parsed, it is returned in the invalid options list.

If you want to raise an exception for all the invalid options, please use
`parse!/2`.
Copy link
Member

Choose a reason for hiding this comment

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

I would remove this addition; in many places we have ! bang variants of functions, but we don't mention them in the documentation for the non-bang variants. Wdyt?


The following extra "types" are supported:

* `:keep` - keeps duplicated items in the list instead of overriding them.
Expand Down Expand Up @@ -117,6 +124,36 @@ defmodule OptionParser do
do_parse(argv, compile_config(opts), [], [], [], true)
end

@doc """
The same as `parse/2` but raises an `OptionParser.InvalidOptionError`
exception if any invalid options are given.
Copy link
Member

Choose a reason for hiding this comment

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

I would make this summary shorter without mentioning the kind of error that is raised (we can mention that in the body of the doc):

The same as parse/2, but raises an exception if there are invalid options.

This is similar to the documentation of other bang variants such as File.cd!/2:

The same as cd/1, but raises an exception if it fails.


If there weren't any errors, returns a three-element tuple as follows:

1. parsed options,
2. remaining arguments,
3. empty list.

## Examples

iex> OptionParser.parse!(["--limit", "xyz"], strict: [limit: :integer])
** (OptionParser.InvalidOptionError) 1 error found! Option --limit is of the wrong type, expected a integer, given "xyz"

iex> OptionParser.parse!(["--unknown", "xyz"], strict: [])
** (OptionParser.InvalidOptionError) 1 error found! Unknown option --unknown

iex> OptionParser.parse!(["-l", "xyz", "-f", "bar"], strict: [limit: :integer, foo: :integer], aliases: [l: :limit, f: :foo])
** (OptionParser.InvalidOptionError) 2 errors found! Option -l is of the wrong type, expected a integer, given "xyz". Option -f is of the wrong type, expected a integer, given "bar"

"""
@spec parse!(argv, options) :: {parsed, argv, errors} | no_return
def parse!(argv, opts \\ []) when is_list(argv) and is_list(opts) do
case parse(argv, opts) do
{_parsed, _argv, []} = result -> result
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this function should return {parsed, argv} without returning any errors since I guess they'd always be [], right?

{_parsed, _argv, errors} -> raise InvalidOptionError, message: format_errors(errors, opts)
end
end

@doc """
Similar to `parse/2` but only parses the head of `argv`;
as soon as it finds a non-switch, it stops parsing.
Expand All @@ -137,14 +174,40 @@ defmodule OptionParser do
do_parse(argv, compile_config(opts), [], [], [], false)
end

@doc """
The same as `parse_head/2` but raises an `OptionParser.InvalidOptionError`
exception if any invalid options are given.

If there weren't any errors, returns a three-element tuple as follows:

1. parsed options,
2. remaining arguments,
3. empty list.

## Examples

iex> OptionParser.parse_head!(["--number", "lib", "test/enum_test.exs", "--verbose"], strict: [number: :integer])
** (OptionParser.InvalidOptionError) 1 error found! Option --number is of the wrong type, expected a integer, given "lib"

iex> OptionParser.parse_head!(["--verbose", "true", "--source", "lib", "test/enum_test.exs", "--unlock"], strict: [verbose: :integer, source: :integer])
** (OptionParser.InvalidOptionError) 2 errors found! Option --verbose is of the wrong type, expected a integer, given "true". Option --source is of the wrong type, expected a integer, given "lib"
"""
@spec parse_head!(argv, options) :: {parsed, argv, errors} | no_return
def parse_head!(argv, opts \\ []) when is_list(argv) and is_list(opts) do
case parse_head(argv, opts) do
{_parsed, _argv, []} = result -> result
{_parsed, _argv, errors} -> raise InvalidOptionError, message: format_errors(errors, opts)
end
end

defp do_parse([], _config, opts, args, invalid, _all?) do
{Enum.reverse(opts), Enum.reverse(args), Enum.reverse(invalid)}
end

defp do_parse(argv, {aliases, switches, strict}=config, opts, args, invalid, all?) do
case next(argv, aliases, switches, strict) do
{:ok, option, value, rest} ->
# the option exist and it was successfully parsed
# the option exists and it was successfully parsed
kinds = List.wrap Keyword.get(switches, option)
new_opts = do_store_option(opts, option, value, kinds)
do_parse(rest, config, new_opts, args, invalid, all?)
Expand Down Expand Up @@ -499,4 +562,28 @@ defmodule OptionParser do
defp negative_number?(arg) do
match?({_, ""}, Float.parse(arg))
end

defp format_errors(errors, opts) do
details = Enum.map_join(errors, ". ", &format_error(&1, opts))
Copy link
Member

@whatyouhide whatyouhide May 18, 2016

Choose a reason for hiding this comment

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

What if we format this so that the output looks like

** (OptionParser.InvalidOptionError) 2 errors found: 

  * option --verbose is of the wrong type, expected a integer, given "true"
  * option --source is of the wrong type, expected a integer, given "lib"

? I think it looks clearer and easier to read and understand.

total = Enum.count(errors)
error = if total == 1, do: "error", else: "errors"
"#{total} #{error} found! #{details}"
Copy link
Member

Choose a reason for hiding this comment

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

I think I mentioned this a while back, but I'm 👎 on the exclamation mark. Now, we even have this recent commit that removed the ! from IO.warn in favour of a more sober colon :).

Copy link
Member

Choose a reason for hiding this comment

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

No worries about those. My plan is to merge the OptionParser related PRs this week and I will fix any inconsistency (specially because those PRs have been blocking on me for a while). :)

Copy link
Member

Choose a reason for hiding this comment

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

@josevalim ah, awesome then. If you're going to merge this PR in the current state, give a look at my comments then :)

end

defp format_error({option, nil}, _) do
"Unknown option #{option}"
end

defp format_error({option, value}, opts) do
option_key = option |> String.lstrip(?-) |> String.to_atom()

type =
if option_alias = opts[:aliases][option_key] do
opts[:strict][option_alias]
else
opts[:strict][option_key]
end

"Option #{option} is of the wrong type, expected a #{type}, given #{inspect value}"
end
end
21 changes: 21 additions & 0 deletions lib/elixir/test/elixir/option_parser_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,27 @@ defmodule OptionParserTest do
== {[source: "from_docs/"], [], [{"--doc", nil}]}
end

test "parse!/2 raise an exception for an unknown option using strict" do
assert_raise OptionParser.InvalidOptionError, "1 error found! Unknown option --doc", fn ->
args = ["--source", "from_docs/", "--doc", "show"]
OptionParser.parse!(args, strict: [source: :string, docs: :string])
end
end

test "parse!/2 raise an exception when an option is of the wrong type" do
assert_raise OptionParser.InvalidOptionError, fn ->
args = ["--bad", "opt", "foo", "-o", "bad", "bar"]
OptionParser.parse!(args, switches: [bad: :integer])
end
end

test "parse_head!/2 raise an exception when an option is of the wrong type" do
assert_raise OptionParser.InvalidOptionError, "1 error found! Option --number is of the wrong type, expected a integer, given \"lib\"", fn ->
args = ["--number", "lib", "test/enum_test.exs"]
OptionParser.parse_head!(args, strict: [number: :integer])
end
end

test ":switches with :strict raises" do
assert_raise ArgumentError, ":switches and :strict cannot be given together", fn ->
OptionParser.parse([], strict: [], switches: [])
Expand Down