Default configuations + Option Normalization #692

eksperimental · 2017-03-08T19:56:58Z

Store in mix.exs the defaults for various options
Add function to convert to nil when an empty string is passes in the command line arguments
Add function to set default arguments in case they are not set

eksperimental · 2017-03-08T20:09:44Z

I am wondering if we should have functions ExDoc.default_config/1-2 that delegate to ExDoc.Mixfile

josevalim · 2017-03-08T20:44:58Z

@eksperimental this is backwards. :) Your code should not depend on the Mixfile. The Mixfile may not even be loaded when the code is running as a dependency.

eksperimental · 2017-03-09T01:13:35Z

@josevalim I sort of remember that we talked about it in the past, and it was when I implemented ExDoc.version/0. So I took the same approach.
Let me know how it looks now

josevalim · 2017-03-09T11:07:39Z

lib/ex_doc.ex

      extras: [],
      filter_prefix: nil,
-      formatter: "html",
+      formatter: Mix.Project.config[:default][:formatter],


This is the same issue. It is bad practice to invoke anything from Mix in your lib code because Mix is a build tool and it may not be available after the software is built. Even though you are using it only at compile time, doing this here means it is very easy for someone to accidentally depend on this at runtime.

What is this PR trying to achieve? Are the values being duplicated somewhere?

If there is duplication, we should handle it on this file exclusively.

sorry, I was too tired yesterday and overlooked this.
Please have a look again at the rebased commit

sourcelevel-bot · 2017-03-09T16:08:08Z

lib/ex_doc.ex

@@ -1,3 +1,20 @@
+defmodule ExDoc.Default do


Modules should have a @moduledoc tag.

We don't need a new module, it can just be in ExDoc. Don't complicate the feature. :)

the reason is that ExDoc module depends on ExDoc.Config, and ExDoc.Config depends on the default config function. I tried creating ExDoc.Config.deafult/0-1 functions an undefined function error. that was why,
If you can find a way that this will work without the extra module, please let me know.. cuz I couldn't

ericmj · 2017-03-09T17:30:05Z

mix.lock

+  "metrics": {:hex, :metrics, "1.0.1", "25f094dea2cda98213cecc3aeff09e940299d950904393b2a29d191c346a8486", [:rebar3], []},
+  "mimerl": {:hex, :mimerl, "1.0.2", "993f9b0e084083405ed8252b99460c4f0563e41729ab42d9074fd5e52439be88", [:rebar3], []},
+  "ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.1", "28a4d65b7f59893bc2c7de786dec1e1555bd742d336043fe644ae956c3497fbe", [:make, :rebar], []},
  "ssl_verify_hostname": {:hex, :ssl_verify_hostname, "1.0.5", "2e73e068cd6393526f9fa6d399353d7c9477d6886ba005f323b592d389fb47be", [:make], []}}


Don't commit mix.lock unless you are changing deps.

@ericmj may I ask why the lock file change if no dependencies have been updated.
and what's the best way to deal with it, use git stash?

@eksperimental Sometimes I do the following:

git update-index --assume-unchanged mix.lock

HTH

@eksperimental It's because a newer version of Hex was used to build the mix.lock.

eksperimental · 2017-04-16T03:22:48Z

ping

josevalim · 2017-04-16T11:04:09Z

I don't believe those changes belong here. If a source can give invalid input, such as empty strings, then those need to be removed and validated at the point of entry and not internally in the code.

josevalim · 2017-04-16T11:05:40Z

lib/ex_doc.ex

+  end

  defp normalize_options(options) do
-    pattern = options[:source_url_pattern] || guess_url(options[:source_url], options[:source_ref] || "master")


This code is doing exactly what the old code does, except the new code is much more verbose. Why change it?

josevalim · 2017-04-16T11:06:05Z

lib/ex_doc.ex

+    end
+  end
+
+  defp normalize_option(options, field, default) do


This function can be written as: options[field] || default. Which is how it was being used before.

Reason was that I was dealing with empty strings as well

eksperimental · 2017-04-25T12:43:05Z

@josevalim Please have a look at the new approach.

whatyouhide · 2017-04-26T21:37:19Z

lib/ex_doc.ex

+
+    @spec config :: map
+    def config,
+      do: @config


I think we tend to avoid the , do: style when there's only one function clause and go with do ... end.

I didn't know about that.

whatyouhide · 2017-04-26T21:38:28Z

lib/ex_doc.ex

+    options =
+      options
+      |> normalize_directory([:assets, :output, :source_root])
+      |> normalize_source_url_pattern


Parens here?

whatyouhide · 2017-04-26T21:41:37Z

lib/ex_doc.ex

-    pattern = options[:source_url_pattern] || guess_url(options[:source_url], options[:source_ref] || "master")
-    options = Keyword.put(options, :source_url_pattern, pattern)
+  def normalize_directory(options, field) when is_atom(field) do
+    if is_bitstring(options[field]) do


Was this meant to be is_binary? 🤔

oh yeah, that's right

eksperimental · 2017-04-26T21:57:58Z

lib/ex_doc.ex

+  end
+
+  @doc """
+  Updates `field` in `options`, if its value is `nil`.


note to myself: remove the comma

Store defaults for various options in ExDoc.Default

…elpers. Replace ExDoc.normalize_options/1 with a more general function named ExDoc.normalize_options/3 Introduce: - ExDoc.nilify_options/1 - ExDoc.normalize_source_url_pattern/2 - ExDoc.normalize_directory/2

eksperimental · 2017-05-06T17:41:04Z

Suggestions addressed

josevalim · 2017-05-11T09:58:58Z

Sorry @eksperimental but I am still having a hard time to understand why this pull request is addressing. Can we break it apart into smaller pull requests? I don't understand why we have to nilify options. If someone passes --foo "" in the command line, why are we nillifying it? Why do we need a whole separate module for keeping defaults? Can't we use module attributes? Maybe we need to break this PR into smaller ones as I can't see the benefits in here after multiple reviews.

eksperimental · 2017-05-13T08:38:10Z

@josevalim I'm addressing your suggestions and submitting two new PRs

josevalim · 2017-05-13T09:58:49Z

Thank you! ❤️

josevalim reviewed Mar 9, 2017

View reviewed changes

eksperimental force-pushed the default_config branch from e12109d to 9a34965 Compare March 9, 2017 16:07

sourcelevel-bot bot reviewed Mar 9, 2017

View reviewed changes

ericmj reviewed Mar 9, 2017

View reviewed changes

eksperimental force-pushed the default_config branch from 9a34965 to afb1a8f Compare March 10, 2017 14:41

josevalim reviewed Apr 16, 2017

View reviewed changes

eksperimental force-pushed the default_config branch from afb1a8f to ad4f841 Compare April 25, 2017 12:39

whatyouhide reviewed Apr 26, 2017

View reviewed changes

eksperimental commented Apr 26, 2017

View reviewed changes

eksperimental added 2 commits May 7, 2017 00:37

Support default configuations

621c0b9

Store defaults for various options in ExDoc.Default

Introduce ExDoc.normalize_options/3, ExDoc.nilify_options/1 and new h…

1c099cd

…elpers. Replace ExDoc.normalize_options/1 with a more general function named ExDoc.normalize_options/3 Introduce: - ExDoc.nilify_options/1 - ExDoc.normalize_source_url_pattern/2 - ExDoc.normalize_directory/2

eksperimental force-pushed the default_config branch from ad4f841 to 1c099cd Compare May 6, 2017 17:38

josevalim closed this May 18, 2017

eksperimental deleted the default_config branch August 19, 2020 07:42

Default configuations + Option Normalization #692

Default configuations + Option Normalization #692

Uh oh!

Conversation

eksperimental commented Mar 8, 2017

Uh oh!

eksperimental commented Mar 8, 2017

Uh oh!

josevalim commented Mar 8, 2017

Uh oh!

eksperimental commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eksperimental commented Apr 16, 2017

Uh oh!

josevalim commented Apr 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eksperimental commented Apr 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eksperimental commented May 6, 2017

Uh oh!

josevalim commented May 11, 2017

Uh oh!

eksperimental commented May 13, 2017

Uh oh!

josevalim commented May 13, 2017

Uh oh!

Uh oh!