Update order of arguments for click/4 + allow title to be optional

[AndroidOatmeal]

Hello, here's my PR for making click/4's title an optional argument.

Due to elixir's rules for applying default arguments, I made the decision to move the http_method default to the end, and moved title \\ nil as the second argument. This is because we will wish to override the title default more often than we wish to override the method default. Now the click/4 can be used in four different ways:

# Defaults: title = nil, method = :get
Pages.click(page, "[data-role=my-data-role]")
# OR
Pages.click(page, data_role: "my-data-role")

# Defaults: method = :get
Pages.click(page, "My Special Link", "[data-role=my-data-role]")
# OR
Pages.click(page, "My Special Link", data_role: "my-data-role")

# Override method, no title selector
Pages.click(page, nil "[data-role=my-data-role]", :post)
# OR
Pages.click(page, nil, [data_role: "my-data-role"], :post)

# No defaults
Pages.click(page, "My Special Link", "[data-role=my-data-role]", :post)
# OR
Pages.click(page, "My Special Link", [data_role: "my-data-role"], :post)

Note that in the last example, we must write our selector as [data_role: "my-data-role"], because the selector is no longer the last argument. In my opinion this tradeoff is worth it; it's probably uncommon to want to specify a title, a selector, and :post. If you can think of a cleaner solution to the ordering/defaults problem, I'm all ears.

[eahanson]

I have an idea, but I won’t have time to try it out until a little later today.

[eahanson]

Changing this (in the top-level Pages module):

  @doc """
  Simulates clicking on an element at `selector` with title `title`.
  Set the `method` param to `:post` to click on a link that has `data-method=post`.
  """
  @spec click(Pages.Driver.t(), http_method(), binary(), Hq.Css.selector()) :: Pages.Driver.t()
  def click(%module{} = page, method \\ :get, title, selector), do: module.click(page, method, title, selector)

to this:

  @doc """
  Simulates clicking on an element at `selector` with title `title`.
  Set the `method` param to `:post` to click on a link that has `data-method=post`.
  """
  @spec click(Pages.Driver.t(), http_method(), binary(), Hq.Css.selector()) :: Pages.Driver.t()
  def click(%module{} = page, method, title, selector), do: module.click(page, method, title, selector)

  @spec click(Pages.Driver.t(), http_method(), Hq.Css.selector()) :: Pages.Driver.t()
  def click(%module{} = page, :get, selector), do: module.click(page, :get, nil, selector)
  def click(%module{} = page, :post, selector), do: module.click(page, :post, nil, selector)

  @spec click(Pages.Driver.t(), binary(), Hq.Css.selector()) :: Pages.Driver.t()
  def click(%module{} = page, title, selector), do: module.click(page, :get, title, selector)

  @spec click(Pages.Driver.t(), Hq.Css.selector()) :: Pages.Driver.t()
  def click(%module{} = page, selector), do: module.click(page, :get, nil, selector)

might work and obviate the need for re-ordering the params. What do you think?

[sax]

I like it. I think we could ship a breaking change if needed, but prefer not to if possible.

The fact that :method is always in [:get, :post] makes it pretty easy to pattern match the different variations.

[sax]

Also is_binary(title) can make things more deterministic.

[eahanson]

Also is_binary(title) can make things more deterministic.

Hmm.... the title can be a string, a Regex, or nil. I guess the current typespec is wrong. Maybe we need to define @type text_filter() :: binary() | Regex.t() | nil

[AndroidOatmeal]

I can take a crack at the proposed changes today

[AndroidOatmeal]

I've added the proposed changes. Take a look and let me know what you think.

[eahanson]

Awesome, thanks Andrew!