Only accept --no-SWITCH for booleans

The previous behaviour was confusing and a special case.
This enables negated switches only for booleans, otherwise
it is considered as a regular switch (as the combination
of --no-SWITCH for any other type is an error anyway).

Author: José Valim

CHANGELOG.md

@@ -46,6 +46,7 @@
   * [Kernel] `exit(integer)` is no longer supported from scripts to configure the exit signal. Use `exit({:shutdown, integer})` instead
   * [Mix] `mix archive` has been renamed to `mix archive.build`
   * [Mix] `Mix.shell.info/1` no longer automatically escape ANSI sequences. Instead if has to be explicitly enabled with the `ansi: true` option
+  * [OptionParser] `--no-SWITCH` are only allowed for declared booleans switches
 
 ## v0.14.2 (2014-06-29)
 

lib/elixir/lib/option_parser.ex

@@ -45,7 +45,7 @@ defmodule OptionParser do
     * `:strict` - the switches are strict. Any switch that does not
       exist in the switch list is treated as an error.
 
-    * `:switches` - configure some switches. Switches that does not
+    * `:switches` - defines some switches. Switches that does not
       exist in the switch list are still attempted to be parsed.
 
   Note only `:strict` or `:switches` may be given at once.
@@ -59,7 +59,7 @@ defmodule OptionParser do
     * `:float`   - parses the switch as a float.
     * `:string`  - returns the switch as a string.
 
-  If a switch can't be parsed or is not specfied in the strict case,
+  If a switch can't be parsed or is not specified in the strict case,
   the option is returned in the invalid options list (third element
   of the returned tuple).
 
@@ -91,14 +91,8 @@ defmodule OptionParser do
 
   ## Negation switches
 
-  All switches starting with `--no-` are considered to be booleans and never
-  parse the next value:
-
-      iex> OptionParser.parse(["--no-op", "path/to/file"])
-      {[no_op: true], ["path/to/file"], []}
-
-  However, in case the base switch exists, it sets that particular switch to
-  false:
+  In case a switch is declared as boolean, it may be passed as `--no-SWITCH`
+  which will set the option to false:
 
       iex> OptionParser.parse(["--no-op", "path/to/file"], switches: [op: :boolean])
       {[op: false], ["path/to/file"], []}
@@ -221,7 +215,7 @@ defmodule OptionParser do
   defp next(["-" <> option|rest], aliases, switches, strict) do
     {option, value} = split_option(option)
     opt_name_bin = "-" <> option
-    tagged = tag_option(option, value, switches, aliases)
+    tagged = tag_option(option, switches, aliases)
 
     if strict and not option_defined?(tagged, switches) do
       {:undefined, opt_name_bin, value, rest}
@@ -296,11 +290,11 @@ defmodule OptionParser do
     end
   end
 
-  defp tag_option(<<?-, option :: binary>>, value, switches, _aliases) do
-    get_negated(option, value, switches)
+  defp tag_option(<<?-, option :: binary>>, switches, _aliases) do
+    get_negated(option, switches)
   end
 
-  defp tag_option(option, _value, _switches, aliases) when is_binary(option) do
+  defp tag_option(option, _switches, aliases) when is_binary(option) do
     opt = get_option(option)
     if alias = aliases[opt] do
       {:default, alias}
@@ -325,23 +319,14 @@ defmodule OptionParser do
     {nil, [:invalid], value}
   end
 
-  defp normalize_option({:negated, option}, nil, switches) do
-    kinds = List.wrap(switches[option])
-
-    cond do
-      :boolean in kinds ->
-        {option, kinds, false}
-      kinds == [] ->
-        {option, kinds, true}
-      true ->
-        {reverse_negated(option), [:invalid], nil}
+  defp normalize_option({:negated, option}, value, switches) do
+    if value do
+      {option, [:invalid], value}
+    else
+      {option, List.wrap(switches[option]), false}
     end
   end
 
-  defp normalize_option({:negated, option}, value, _switches) do
-    {option, [:invalid], value}
-  end
-
   defp normalize_option({:default, option}, value, switches) do
     {option, List.wrap(switches[option]), value}
   end
@@ -396,24 +381,18 @@ defmodule OptionParser do
     end
   end
 
-  defp reverse_negated(negated) do
-    String.to_atom("no_" <> Atom.to_string(negated))
-  end
-
-  defp get_negated("no-" <> rest = option, value, switches) do
-    if negated = get_option(rest) do
-      option = if Keyword.has_key?(switches, negated) and value == nil do
-        negated
-      else
-        get_option(option)
-      end
-      {:negated, option}
-    else
-      :unknown
+  defp get_negated("no-" <> rest = original, switches) do
+    cond do
+      (negated = get_option(rest)) && :boolean in List.wrap(switches[negated]) ->
+        {:negated, negated}
+      option = get_option(original) ->
+        {:default, option}
+      true ->
+        :unknown
     end
   end
 
-  defp get_negated(rest, _value, _switches) do
+  defp get_negated(rest, _switches) do
     if option = get_option(rest) do
       {:default, option}
     else

lib/elixir/test/elixir/option_parser_test.exs

@@ -48,7 +48,7 @@ defmodule OptionParserTest do
     assert OptionParser.parse(["--no-bool"], strict: [])
            == {[], [], [{"--no-bool", nil}]}
     assert OptionParser.parse(["--no-bool=...", "other"])
-           == {[], ["other"], [{"--no-bool", "..."}]}
+           == {[no_bool: "..."], ["other"], []}
   end
 
   test "does not parse -- as an alias" do
@@ -103,7 +103,7 @@ defmodule OptionParserTest do
     assert OptionParser.parse(["--value"], switches: [value: :string])
            == {[], [], [{"--value", nil}]}
     assert OptionParser.parse(["--no-value"], switches: [value: :string])
-           == {[], [], [{"--no-value", nil}]}
+           == {[no_value: true], [], []}
   end
 
   test "parses configured integers" do
@@ -134,11 +134,6 @@ defmodule OptionParserTest do
            == {[], ["foo"], [{"--value", "WAT"}]}
   end
 
-  test "parses no switches as flags" do
-    assert OptionParser.parse(["--no-docs", "foo"])
-           == {[no_docs: true], ["foo"], []}
-  end
-
   test "overrides options by default" do
     assert OptionParser.parse(["--require", "foo", "--require", "bar", "baz"])
            == {[require: "bar"], ["baz"], []}
@@ -157,17 +152,17 @@ defmodule OptionParserTest do
   end
 
   test "stops on --" do
-    options = OptionParser.parse(["--source", "from_docs/", "--", "1", "2", "3"])
-    assert options == {[source: "from_docs/"], ["1", "2", "3"], []}
+    options = OptionParser.parse(["--source", "foo", "--", "1", "2", "3"])
+    assert options == {[source: "foo"], ["1", "2", "3"], []}
 
-    options = OptionParser.parse_head(["--source", "from_docs/", "--", "1", "2", "3"])
-    assert options == {[source: "from_docs/"], ["1", "2", "3"], []}
+    options = OptionParser.parse_head(["--source", "foo", "--", "1", "2", "3"])
+    assert options == {[source: "foo"], ["1", "2", "3"], []}
 
-    options = OptionParser.parse(["--no-dash", "foo", "bar", "--", "-x"])
-    assert options == {[no_dash: true], ["foo", "bar", "-x"], []}
+    options = OptionParser.parse(["--source", "foo", "bar", "--", "-x"])
+    assert options == {[source: "foo"], ["bar", "-x"], []}
 
-    options = OptionParser.parse_head(["--no-dash", "foo", "bar", "--", "-x"])
-    assert options == {[no_dash: true], ["foo", "bar", "--", "-x"], []}
+    options = OptionParser.parse_head(["--source", "foo", "bar", "--", "-x"])
+    assert options == {[source: "foo"], ["bar", "--", "-x"], []}
   end
 
   test "goes beyond the first non option arguments" do
@@ -288,6 +283,6 @@ defmodule OptionParserTest do
     assert OptionParser.next(["--bool=", "..."], config)
            == {:invalid, "--bool", "", ["..."]}
     assert OptionParser.next(["--no-bool=", "..."], config)
-           == {:undefined, "--no-bool", "", ["..."]}
+           == {:invalid, "--no-bool", "", ["..."]}
   end
 end

lib/mix/lib/mix/tasks/app.start.ex

@@ -18,14 +18,13 @@ defmodule Mix.Tasks.App.Start do
 
   """
   def run(args) do
-    {opts, _, _} = OptionParser.parse(args)
     Mix.Task.run "loadpaths", ["--no-readd"|args]
 
-    unless opts[:no_compile] do
+    unless "--no-compile" in args do
       Mix.Task.run "compile", ["--no-readd"|args]
     end
 
-    unless opts[:no_start] do
+    unless "--no-start" in args do
       start(Mix.Project.config[:app])
     end
 

lib/mix/lib/mix/tasks/archive.build.ex

@@ -33,11 +33,11 @@ defmodule Mix.Tasks.Archive.Build do
 
   def run(args) do
     {opts, _, _} = OptionParser.parse(args, aliases: [o: :output, i: :input],
-                                      switches: [force: :boolean, no_compile: :boolean])
+                                      switches: [force: :boolean, compile: :boolean])
 
     project = Mix.Project.get
 
-    if project && !opts[:no_compile] do
+    if project && Keyword.get(opts, :compile, true) do
       Mix.Task.run :compile, args
     end
 

lib/mix/lib/mix/tasks/compile.elixir.ex

@@ -17,13 +17,11 @@ defmodule Mix.Tasks.Compile.Elixir do
 
   ## Command line options
 
-    * `--force`         - forces compilation regardless of modification times
-    * `--no-docs`       - do not attach documentation to compiled modules
-    * `--no-debug-info` - do not attach debug info to compiled modules
-    * `--ignore-module-conflict`
-                        - do not emit warnings if a module was previously defined
-    * `--warnings-as-errors`
-                        - treat warnings as errors and return a non-zero exit code
+    * `--force` - forces compilation regardless of modification times
+    * `--docs` (`--no-docs`) - attach (or not) documentation to compiled modules
+    * `--debug-info` (`--no-debug-info`) - attach (or not) debug info to compiled modules
+    * `--ignore-module-conflict` - do not emit warnings if a module was previously defined
+    * `--warnings-as-errors` - treat warnings as errors and return a non-zero exit code
 
   ## Configuration
 

lib/mix/lib/mix/tasks/deps.check.ex

@@ -2,7 +2,7 @@ defmodule Mix.Tasks.Deps.Check do
   use Mix.Task
 
   import Mix.Dep, only: [loaded: 1, loaded_by_name: 2, format_dep: 1,
-                          format_status: 1, check_lock: 2, ok?: 1]
+                         format_status: 1, check_lock: 2, ok?: 1]
 
   @moduledoc """
   Checks if all dependencies are valid and if not, abort.
@@ -17,7 +17,6 @@ defmodule Mix.Tasks.Deps.Check do
 
   """
   def run(args) do
-    {opts, _, _} = OptionParser.parse(args)
     lock = Mix.Dep.Lock.read
     all  = Enum.map(loaded(env: Mix.env), &check_lock(&1, lock))
 
@@ -27,10 +26,10 @@ defmodule Mix.Tasks.Deps.Check do
     cond do
       not_ok != [] ->
         show_not_ok(not_ok)
-      compile == [] or opts[:no_compile] ->
+      compile == [] or "--no-compile" in args ->
         :ok
       true ->
-        Mix.Tasks.Deps.Compile.compile(compile, opts)
+        Mix.Tasks.Deps.Compile.compile(compile)
         show_not_ok compile
                     |> Enum.map(& &1.app)
                     |> loaded_by_name(env: Mix.env)

lib/mix/lib/mix/tasks/deps.compile.ex

@@ -31,15 +31,15 @@ defmodule Mix.Tasks.Deps.Compile do
     Mix.Project.get! # Require the project to be available
 
     case OptionParser.parse(args) do
-      {opts, [], _} ->
-        compile(Enum.filter(loaded(env: Mix.env), &compilable?/1), opts)
-      {opts, tail, _} ->
-        compile(loaded_by_name(tail, env: Mix.env), opts)
+      {_, [], _} ->
+        compile(Enum.filter(loaded(env: Mix.env), &compilable?/1))
+      {_, tail, _} ->
+        compile(loaded_by_name(tail, env: Mix.env))
     end
   end
 
   @doc false
-  def compile(deps, _opts) do
+  def compile(deps) do
     shell  = Mix.shell
     config = Mix.Project.deps_config
 

lib/mix/lib/mix/tasks/escript.build.ex

@@ -72,12 +72,12 @@ defmodule Mix.Tasks.Escript.Build do
 
   """
   def run(args) do
-    {opts, _, _} = OptionParser.parse(args, switches: [force: :boolean, no_compile: :boolean])
+    {opts, _, _} = OptionParser.parse(args, switches: [force: :boolean, compile: :boolean])
 
     # Require the project to be available
     Mix.Project.get!
 
-    unless opts[:no_compile] do
+    if Keyword.get(opts, :compile, true) do
       Mix.Task.run :compile, args
     end
 

lib/mix/lib/mix/tasks/run.ex

@@ -36,7 +36,8 @@ defmodule Mix.Tasks.Run do
   def run(args) do
     {opts, head, _} = OptionParser.parse_head(args,
       aliases: [r: :require, pr: :parallel_require, e: :eval, c: :config],
-      switches: [parallel_require: :keep, require: :keep, eval: :keep, config: :keep])
+      switches: [parallel_require: :keep, require: :keep, eval: :keep, config: :keep,
+                 halt: :boolean, compile: :boolean, deps_check: :boolean, start: :boolean])
 
     # Require the project to be available
     Mix.Project.get!
@@ -63,7 +64,8 @@ defmodule Mix.Tasks.Run do
         Mix.raise "No such file: #{file}"
       end
     end
-    if opts[:no_halt], do: :timer.sleep(:infinity)
+
+    unless Keyword.get(opts, :halt, true), do: :timer.sleep(:infinity)
   end
 
   defp process_config(opts) do