Introduce ExDoc.normalize_options/3 and new helpers #709

eksperimental · 2017-05-15T03:35:39Z

Replace ExDoc.normalize_options/1 with a more general function named ExDoc.normalize_options/3

Introduce:

ExDoc.normalize_source_url_pattern/2
ExDoc.normalize_directory/2

josevalim · 2017-05-15T08:03:28Z

Thanks @eksperimental!

Replace ExDoc.normalize_options/1 with a more general function named ExDoc.normalize_options/3

Why? What are we solving with this? Which issues are we trying to prevent? What do we gain? :)

eksperimental · 2017-05-18T21:34:11Z

I will report back later here, as I have found other issues, and I would like to sort that first and then see what's the best approach here.
Let's hold it off for now until we sort the other issue

sourcelevel-bot · 2017-05-19T14:52:32Z

Ebert has finished reviewing this Pull Request and has found:

1 fixed issue! 🎉

You can see more details about this review at https://ebertapp.io/github/elixir-lang/ex_doc/pulls/709.

eksperimental · 2017-05-19T15:55:03Z

@josevalim, view check the new changes.
After reworking it out for a while I ended up leaving normalize_options/1 and adding normalize_options/3.

The reason for introducing normalize_options/3 is because it allows us to modify options directly without the need to create new functions and duplicate code as in https://github.com/elixir-lang/ex_doc/pull/709/files#diff-817174a6c3600e5759b5cf9117b0f39fL164

milmazz

@eksperimental I left a few comments.

milmazz · 2017-05-19T18:23:30Z

lib/ex_doc.ex

+  def normalize_options(options) do
+    options = normalize_options(options, :source_ref, ExDoc.Config.default(:source_ref))
+
+    options


What about this option:

options |> normalize_options(:source_ref, ExDoc.Config.default(:source_ref)) |> normalize_directory([:assets, :output, :source_root]) |> normalize_source_url_pattern(options[:source_ref] || ExDoc.Config.default(:source_ref))

nope. this was done intentionally.
You cannot do that, because in the last line when I access options[:source_ref] I need the updated version that normalize_options($options, :source_ref, ExDoc.Config.default(:source_ref)) updates

milmazz · 2017-05-19T18:26:33Z

lib/mix/tasks/docs.ex

-
-    if args != [] do
-      Mix.raise "Extraneous arguments on the command line"
+    {cli_options, remaining_args, _invalid} =


This section does not seems to be related with your original goal, which is add ExDoc.normalize_options/3.

milmazz · 2017-05-19T18:33:30Z

lib/mix/tasks/docs.ex

-      |> Keyword.merge(cli_opts)
-      |> normalize_source_url(config)   # accepted at root level config
-      |> normalize_homepage_url(config) # accepted at root level config
+      # CLI options have presedence over config


s/presedence/precedence/

milmazz · 2017-05-19T18:38:22Z

lib/mix/tasks/docs.ex

+      # CLI options have presedence over config
+      |> Keyword.merge(cli_options)
+      # source_url is accepted at root level config
+      |> (&(if value = config[:source_url],


At least for me, this anonymous function trick brakes the normal workflow of the data transformation. I like the way it was before, normalize_source_url/1 and normalize_homepage_url/1 are explicit and show their intention without looking the implementation details, if you want to look into the details you can go straight to that function. wdyt?

yes. that's correct. I will use those then.

milmazz · 2017-05-19T18:39:43Z

test/ex_doc/formatter/html_test.exs

-    assert "#{output_dir()}/another_dir/dist/app-*.css" |> Path.wildcard |> File.regular?
-    assert "#{output_dir()}/another_dir/dist/app-*.js" |> Path.wildcard |> File.regular?
+    for file <- Path.wildcard("#{output_dir()}/another_dir/dist/app-*.{js,css}") do
+      assert File.regular?(file) == true


You don't need the == true at the end, right?

that's the convention in the Elixir project. I probably got it from there.

I don't think it is a convention in Elixir. For example, the tests for File.regular?/1 (or for File in general for that matter) never use it.

you are right.
It is used only with similar lines, and the value will be true or false.

as in

assert ("abcd" =~ ~R/c(d)/) == true assert ("abcd" =~ ~R/e/) == false

Even that should be rewritten to

assert "abcd" =~ ~R/.../ refute ...

so that ExUnit can potentially produce a diff in case of failure. The only reason to use == true (or false) as far as I know is when the function can return something else than a boolean and you want to assert that the specific case you're testing returns a boolean (for example assert foo would pass if foo was anything non-nil and non-false).

josevalim · 2017-05-19T19:13:45Z

The reason for introducing normalize_options/3 is because it allows us to modify options directly without the need to create new functions and duplicate code

I don't understand, if this change is about duplicate code, why is it adding more lines of code than removing? Sorry but I really can't see what we are gaining with those changes.

eksperimental · 2017-05-20T05:45:09Z

Thank you for the review @milmazz,
I have implement most of your suggestions, please have a look one last time.
I have reworked the helper so it doesn't break the pipe line flow.

@josevalim If you are still not convinced,
please close the PR.

Introduce: - ExDoc.normalize_source_url_pattern/2 - ExDoc.normalize_paths/2

josevalim · 2017-05-20T07:30:43Z

Ok, I will close the PR then. Thank you @eksperimental for the different attempts.

josevalim · 2017-05-20T09:37:09Z

We may check for explicit bools in Elixir because we may be checking that =~ returns a Boolean. Other than that, there is no reason for a explicit book check. -- *José Valimwww.plataformatec.com.br <http://www.plataformatec.com.br/>Founder and Director of R&D*

eksperimental force-pushed the default_config3 branch from 47ce3ca to 625ad2f Compare May 15, 2017 03:59

eksperimental force-pushed the default_config3 branch from 625ad2f to 7d7446d Compare May 19, 2017 14:51

eksperimental force-pushed the default_config3 branch from 7d7446d to 711777d Compare May 19, 2017 14:57

milmazz requested changes May 19, 2017

View reviewed changes

Fix test in Formatter.HTMLTest

29399a9

eksperimental force-pushed the default_config3 branch from 19722d8 to c0d192d Compare May 20, 2017 05:39

Introduce ExDoc.normalize_options/3 and new helpers.

2ad6df3

Introduce: - ExDoc.normalize_source_url_pattern/2 - ExDoc.normalize_paths/2

eksperimental force-pushed the default_config3 branch from c0d192d to 2ad6df3 Compare May 20, 2017 05:47

josevalim closed this May 20, 2017

eksperimental deleted the default_config3 branch May 20, 2017 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ExDoc.normalize_options/3 and new helpers #709

Introduce ExDoc.normalize_options/3 and new helpers #709

eksperimental commented May 15, 2017

josevalim commented May 15, 2017

eksperimental commented May 18, 2017

sourcelevel-bot bot commented May 19, 2017

eksperimental commented May 19, 2017 •

edited

milmazz left a comment

milmazz May 19, 2017

eksperimental May 20, 2017 •

edited

milmazz May 19, 2017

milmazz May 19, 2017

milmazz May 19, 2017

eksperimental May 20, 2017

milmazz May 19, 2017

eksperimental May 20, 2017

whatyouhide May 20, 2017 •

edited

eksperimental May 20, 2017

whatyouhide May 20, 2017

josevalim commented May 19, 2017

eksperimental commented May 20, 2017

josevalim commented May 20, 2017

josevalim commented May 20, 2017 via email

Introduce ExDoc.normalize_options/3 and new helpers #709

Introduce ExDoc.normalize_options/3 and new helpers #709

Conversation

eksperimental commented May 15, 2017

josevalim commented May 15, 2017

eksperimental commented May 18, 2017

sourcelevel-bot bot commented May 19, 2017

eksperimental commented May 19, 2017 • edited

milmazz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eksperimental May 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whatyouhide May 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented May 19, 2017

eksperimental commented May 20, 2017

josevalim commented May 20, 2017

josevalim commented May 20, 2017 via email

eksperimental commented May 19, 2017 •

edited

eksperimental May 20, 2017 •

edited

whatyouhide May 20, 2017 •

edited