Fix ExDoc.CLI.main/2 to keep --source-ref #680

eksperimental · 2017-03-03T03:45:10Z

--source-ref passed to the CLI was being lost,
meaning all links to commits/tags were being lost and the docs were always
linked to master

sourcelevel-bot · 2017-03-03T03:45:11Z

Hello, @eksperimental! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

sourcelevel-bot · 2017-03-03T03:58:20Z

lib/ex_doc/issue446.ex

+  @source_root File.cwd!
+
+  def run(relative_path) do
+    ExDoc.Retriever.docs_from_dir(relative_path, relative_path |> doc_config)


Use a function call when a pipeline is only one function long

sourcelevel-bot · 2017-03-03T03:58:20Z

lib/ex_doc/issue446.ex

+      source_beam: Path.expand relative_path
+    }
+  end
+end


There should be a final \n at the end of each file.

sourcelevel-bot · 2017-03-03T03:58:20Z

lib/ex_doc/issue446.ex

@@ -0,0 +1,17 @@
+# file: lib/ex_doc/issue446.ex
+defmodule ExDoc.Issue446 do


Modules should have a @moduledoc tag.

milmazz

@eksperimental Thanks! I just leave some comments.

milmazz · 2017-03-03T04:00:31Z

lib/ex_doc/cli.ex

-                         e: :extra, v: :version, n: :canonical, s: :extra_section,
-                         a: :assets, i: :filter_prefix],
-               switches: [extra: :keep])
+    {opts, args, _invalid} = OptionParser.parse(args,


Why not something like this?

{opts, args, _invalid} = OptionParser.parse(args, ...

milmazz · 2017-03-03T04:01:13Z

lib/ex_doc/cli.ex

+        a: :assets,
+        l: :logo,
+        n: :canonical,
+        v: :version,


WDYT about sorting this aliases?

nope. the idea (as the title of the commit suggests) is to keep the same order as the exdoc --help command lists them

Ok, I see, then, It would be nice to see an inline note about your suggestion because it's easy to see the pattern when those aliases are ordered alphabetically, but no when they are listed in same order as the docs, wdyt?

milmazz · 2017-03-03T04:02:20Z

lib/ex_doc/issue446.ex

@@ -0,0 +1,17 @@
+# file: lib/ex_doc/issue446.ex


Do you really want to include this file?

nope. it was a mistake. it's from a really old PR

milmazz · 2017-03-03T04:03:59Z

test/ex_doc/cli_test.exs

+      ]
+
+    {project, version, opts} =
+      run(args)


Why not something like this:

{project, version, opts} = run(args)

oops. that was not intented

--source-ref passed to the CLI was being lost, meaning all links to commits/tags were being lost and the docs were always linked to master

eksperimental · 2017-03-03T04:15:33Z

@milmazz thank your for your input. please have a look again.

milmazz

@eksperimental Sure!

milmazz · 2017-03-03T04:24:30Z

lib/ex_doc/cli.ex

+
+      switches: [
+        extra: :keep,
+      ]


Missing comma after ]

it will be weird to have a ) after the comma, ex: switches: [...], )

milmazz · 2017-03-03T04:27:58Z

lib/ex_doc/cli.ex

+        a: :assets,
+        l: :logo,
+        n: :canonical,
+        v: :version,


Ok, I see, then, It would be nice to see an inline note about your suggestion because it's easy to see the pattern when those aliases are ordered alphabetically, but no when they are listed in same order as the docs, wdyt?

milmazz · 2017-03-03T04:30:36Z

lib/ex_doc.ex

      output: "doc",
      project: nil,
      retriever: ExDoc.Retriever,
      source_beam: nil,


WDYT about removing those nil from defstruct/1?

actually, I was thinking of that. should we use an empty string?

The default value is nil, so, you can have something like this:

defstruct [ :main, :project, output: "doc", retriever: ExDoc.Retriever ]

but what's the point since those are not required.

oh. I see now what you mean.
I was thinking why not have a default empty string.
I will be submitting another PR, and because we are using nil I need to check agains nil, false or "" to see if an option is unset. If by default we set to to empty, we just need to check against false or "" (and false is because people may have just used --foo without value, and it because a boolean argv.

I will be submitting another PR, and because we are using nil I need to check agains nil, false or "" to see if an option is unset.

We should continue only checking for nil. In Elixir, nil is used for absence of information (except if we have a field that may be boolean).

If the field can be an empty string and an empty string also mean "emptiness", we need to convert the empty string to nil before putting it in the struct.

I have another PR that is waiting for this one to be merged, and it deals with normalizing options individually, I guess I can include this one rule

If the field can be an empty string and an empty string also mean "emptiness", we need to convert the empty string to nil before putting it in the struct.

Yes, we should preprocess those empty strings and convert the data to nil. Also, it looks better doing this:

if options.string_field do # ... end

than this:

if options.string_field != "" do # ... end

:)

josevalim · 2017-03-03T09:59:53Z

lib/ex_doc.ex

      project: nil,
      retriever: ExDoc.Retriever,
      source_beam: nil,
+      source_ref: "master",


I don't understand why this is needed. :source_ref is not needed in the struct inside ExDoc. We only use it initially when building the source_url.

shouldn't the struct reflect what can be passed via config, such as CLI or the docs task?

ex_doc/lib/mix/tasks/docs.ex

Line 62 in 05eb89e

* `:source_url_pattern` - Public URL of the project. Derived from

There is no need to keep the source_ref around if we don't use it internally. The goal of ExDoc.build_config is build a set of options and build a struct that is relevant internally, if we don't use source_ref past building the config, there is no point in keeping it in the struct.

Ok, I guess if we ever need to find out what was passed by the user, we can add it later to it

josevalim · 2017-03-03T10:00:27Z

lib/ex_doc/cli.ex

+
+        switches: [
+          extra: :keep,
+          source_ref: :string,


Same here. This is not needed.

This is the line that was missing and that caused source_ref to be ditched. So I would say it is a total must to be here.

Ah, it is missing it because of the changes in Elixir v1.4 regarding atoms.

It would be best for us to use :strict then and define all inputs. We can even move to use parse! so we get proper argument handling.

eksperimental · 2017-03-03T14:49:28Z

lib/ex_doc.ex


     @type t :: %__MODULE__{
       canonical: nil | String.t,
       deps: [{ebin_path :: String.t, doc_url :: String.t}],


@josevalim according to what you said, I am uncertain whether these typespecs should be nil | String.t or just String.t as they are.

josevalim · 2017-03-03T20:16:12Z

I have merged this right now so we get proper docs but further improvements based on the comments are still welcome. :)

Sort args in ExDoc.CLI so they are listed in same order as documentation

3395888

eksperimental force-pushed the source_ref2 branch from b1fcc68 to 1ba0620 Compare March 3, 2017 03:57

sourcelevel-bot bot reviewed Mar 3, 2017

View reviewed changes

milmazz requested changes Mar 3, 2017

View reviewed changes

Fix ExDoc.CLI.main/2 to keep --source-ref

ea6cf10

--source-ref passed to the CLI was being lost, meaning all links to commits/tags were being lost and the docs were always linked to master

eksperimental force-pushed the source_ref2 branch from 1ba0620 to ea6cf10 Compare March 3, 2017 04:15

milmazz reviewed Mar 3, 2017

View reviewed changes

josevalim reviewed Mar 3, 2017

View reviewed changes

Correct typespecs in %ExDoc.Config{}

489ff04

eksperimental commented Mar 3, 2017

View reviewed changes

Remove :source_ref from %ExDoc.Config{}

434644f

josevalim merged commit 2228073 into elixir-lang:master Mar 3, 2017

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

		@@ -0,0 +1,17 @@
		# file: lib/ex_doc/issue446.ex
		defmodule ExDoc.Issue446 do

Fix ExDoc.CLI.main/2 to keep --source-ref #680

Fix ExDoc.CLI.main/2 to keep --source-ref #680

Uh oh!

Conversation

eksperimental commented Mar 3, 2017

Uh oh!

sourcelevel-bot bot commented Mar 3, 2017

Uh oh!

sourcelevel-bot bot Mar 3, 2017

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Mar 3, 2017

Choose a reason for hiding this comment

Uh oh!

sourcelevel-bot bot Mar 3, 2017

Choose a reason for hiding this comment

Uh oh!

milmazz left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eksperimental commented Mar 3, 2017

Uh oh!

milmazz left a comment

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!

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!

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 Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

eksperimental Mar 3, 2017 •

edited

Loading

eksperimental Mar 3, 2017 •

edited

Loading