Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make mix help list aliases #7805

Merged
merged 10 commits into from Jun 30, 2018
Merged

Conversation

@Sihui
Copy link
Contributor

@Sihui Sihui commented Jun 27, 2018

Sihui added 3 commits Jun 27, 2018
@Sihui
Copy link
Contributor Author

@Sihui Sihui commented Jun 27, 2018

Not sure why the latest build failed: https://travis-ci.org/elixir-lang/elixir/jobs/397193995.
The second latest build also failed but on a different test: https://travis-ci.org/elixir-lang/elixir/jobs/397182429. 😞

Does the new code introduce something flaky? 😨

@smorin
Copy link
Contributor

@smorin smorin commented Jun 27, 2018

I was just wanting this exact functionality. Thanks for doing the pull request.

@michalmuskala
Copy link
Member

@michalmuskala michalmuskala commented Jun 27, 2018

The build failure seems unrelated. I rested the failed suite.


defp task_str(task_name) when is_list(task_name) do
task_strs = Enum.map(task_name, &task_str/1) |> Enum.join(", ")
"[" <> task_strs <> "]"

This comment has been minimized.

@michalmuskala

michalmuskala Jun 27, 2018
Member

I wonder about the format here with the list, but I'm not really sure what would be better.

This comment has been minimized.

@josevalim

josevalim Jun 27, 2018
Member

I would say we should just call inspect. It is the simplest implementation and it would be similar to what we have today. But I am not sure.

I guess the formatting was the biggest concern for including aliases in the mix help, as they can get quite verbose. Another option is to just have a simple text: "# Alias defined in mix.exs" and call it a day.

Thoughts?

@fertapric
Copy link
Member

@fertapric fertapric commented Jun 27, 2018

I think in order for this feature to be complete, this PR should supportmix help ALIAS. It might be confusing to be able to run mix help TASK on some of the names listed but not on others.

This might also simplify the formatting of the aliases in mix help. As @josevalim suggested, mix help can just display my_alias # Alias defined in mix.exs; and mix help my_alias could display a more verbose description:

Alias for ["run my/really/long/path.exs", "cmd another_long_cmd"]

Thoughts?

@michalmuskala
Copy link
Member

@michalmuskala michalmuskala commented Jun 27, 2018

I like this solution.

@smorin
Copy link
Contributor

@smorin smorin commented Jun 27, 2018

I like: my_alias # Alias defined in mix.exs as a solution but why not also allow.

defp aliases do
    [
      help_make: [alias: "cmd make", description: "my description", new_option: "asdf"]
    ]
  end

OR

defp aliases do
    [
      help_make: %{alias: "cmd make", description: "my description", new_option: "asdf"}
    ]
  end
@josevalim
Copy link
Member

@josevalim josevalim commented Jun 27, 2018

@smorin If you want to add descriptions and what not, then I would say you should just go ahead and define a proper task.

@smorin
Copy link
Contributor

@smorin smorin commented Jun 27, 2018

@josevalim Why force someone to write a task for something that takes 3 words. It's using a sledge hammer to drive a nail. Especially in cases, where the command is 2 or 3 separate words and the description is the longest part. Why clutter a code base, adding steps for a developer, and more lines of code to maintain.

npm does it will good effect: https://docs.npmjs.com/misc/scripts

{docs, max} = build_task_doc_list(modules)
aliases =
load_aliases()
|> Enum.filter(&String.contains?(elem(&1, 0), pattern))

This comment has been minimized.

@whatyouhide

whatyouhide Jun 27, 2018
Member

We try to avoid single pipes. I would suggest to refactor this with a for comprehension:

aliases = 
  for {alias_name, _} = alias <- load_aliases(),
      String.contains?(alias_name, pattern),
      do: alias

This comment has been minimized.

@Sihui

Sihui Jun 27, 2018
Author Contributor

why do we try to avoid single pipes?

This comment has been minimized.

@josevalim

josevalim Jun 27, 2018
Member

@Sihui mostly because if we don't have multiple expressions, the pipeline ends up being more noise than helping readability. We know it is subjective but that's why we decided to add a "rule", so it is a clear path and we remove subjectivity. :)

This comment has been minimized.

@Sihui

Sihui Jun 27, 2018
Author Contributor

@josevalim
A (weak) counter argument will be using single pipe makes adding additional pipe easier. But that's for another discussion.

I don't think we should avoid Enum.filter b/c of avoiding single pipes though. I think Enum.filter expresses our intention clearer than the for comprehension. How about

Enum.filter(load_aliases, &String.contains?(elem(&1, 0), pattern))

A side note, we have quite a few usages of single pipes throughout the file (and the codebase), does it mean we want to replace all of them?

This comment has been minimized.

@whatyouhide

whatyouhide Jun 27, 2018
Member

Eventually we want to replace all pipes, but we try to avoid small changes like that in the codebase in order to keep the number of changes down. Enum.filter/2 is also very much fine with me, I just don't love elem/2 since it hides what the tuple looks like. What about:

Enum.filter(load_aliases(), fn {name, _} -> String.contains?(name, pattern) end)

This comment has been minimized.

@Sihui

Sihui Jun 27, 2018
Author Contributor

good call! I not a fan of elem either.

@@ -116,6 +122,11 @@ defmodule Mix.Tasks.Help do
|> Enum.filter(&(Mix.Task.moduledoc(&1) != false))
end

defp load_aliases() do
Mix.Project.config()[:aliases]
|> Enum.map(fn {k, v} -> {Atom.to_string(k), v} end)

This comment has been minimized.

@whatyouhide

whatyouhide Jun 27, 2018
Member

We try to avoid single pipes and single-letter variables. Maybe rewrite this as:

defp load_aliases() do
  for {name, value} <- Mix.Project.config()[:aliases] do
    {Atom.to_string(name), value}
  end
end

This comment has been minimized.

@Sihui

Sihui Jun 27, 2018
Author Contributor

updated to

  defp load_aliases() do
    Mix.Project.config()[:aliases]
    |> Enum.map(fn {k, v} -> {Atom.to_string(k), v} end)
    |> Map.new()
  end

to make verbose_doc easier. (turning it to a map also makes more sense.)

This comment has been minimized.

@fertapric

fertapric Jun 27, 2018
Member

What do you think about these names to address the single-letter variables?

defp load_aliases() do
  Mix.Project.config()[:aliases]
  |> Enum.map(fn {alias_name, alias_tasks} -> {Atom.to_string(alias_name), alias_tasks} end)
  |> Map.new()
end

This comment has been minimized.

@whatyouhide

whatyouhide Jun 27, 2018
Member

You're doing two passes this way. I didn't know you needed a map but if you do, I'd much rather go with into: %{} in the for comprehension or just Map.new/2:

aliases = Mix.Project.config()[:aliases]
Map.new(aliases, fn {alias_name, alias_tasks} -> {Atom.to_string(alias_name), alias_tasks} end)

(also, definitely let's avoid single-letter variables!)

This comment has been minimized.

@Sihui

Sihui Jun 27, 2018
Author Contributor

@whatyouhide I used map b/c I couldn't find an easy way to accomplish Map.has_key?(aliases, task). If there's a way without having to use map, I would love to learn that! :)

"[" <> task_strs <> "]"
end

defp task_str(task_name) when is_bitstring(task_name), do: task_name

This comment has been minimized.

@whatyouhide

whatyouhide Jun 27, 2018
Member

Instead of is_bitstring/1, maybe use is_binary/1 here?

@josevalim
Copy link
Member

@josevalim josevalim commented Jun 27, 2018

@josevalim Why force someone to write a task for something that takes 3 words. It's using a sledge hammer to drive a nail.

I would say that ad-hoc aliases would end-up being more clutter than well-structured tasks but I see your point. We should move the discussion about revamping aliases elsewhere then. Maybe a proposal to the elixir-lang-core mailing list as the scope of this particular issue is well defined based on how aliases work today and we should not go off-topic.

assert output =~ "# mix nested.h\n\n"

assert output =~
~r/Alias for \[#Function<.*\/1 in .*ComplexAliases.project\/0>, \"h foo bar\"\]\n/

This comment has been minimized.

@Sihui

Sihui Jun 27, 2018
Author Contributor

Couldn't find an easy way to format "nested.h": [&Mix.shell().info(inspect(&1)), "h foo bar"] nicely.
Thoughts?

This comment has been minimized.

@josevalim

josevalim Jun 27, 2018
Member

@Sihui this is honestly good enough to me. :)

@Sihui
Copy link
Contributor Author

@Sihui Sihui commented Jun 27, 2018

^^ Updated to:

  1. use my_alias # Alias defined in mix.exs in mix help
  2. and add support for mix help ALIAS
## Arguments
mix help - prints all tasks and their shortdoc
mix help - prints all aliases, tasks and task shortdoc

This comment has been minimized.

@fertapric

fertapric Jun 27, 2018
Member

I think there is a typo here "prints all aliases, tasks and their shortdoc"

This comment has been minimized.

@Sihui

Sihui Jun 27, 2018
Author Contributor

I don't think we have shortdoc for aliases. shortdoc are marked with @shortdoc

This comment has been minimized.

@fertapric

fertapric Jun 27, 2018
Member

"and their summary" or "and their short description"?

@@ -4,11 +4,12 @@ defmodule Mix.Tasks.Help do
@shortdoc "Prints help information for tasks"

@moduledoc """
Lists all tasks or prints the documentation for a given task.
Lists all aliases and tasks or prints the documentation for a given task.

This comment has been minimized.

@fertapric

fertapric Jun 27, 2018
Member

Minor nitpick: I would change the order of "tasks" and "aliases" here (and below). For some reason, in my head 😅 , it makes more sense to put "tasks" at the beginning as they are more relevant for this specific task :) So, "Lists all tasks and aliases or prints the documentation for a given task or alias."

@@ -29,6 +29,20 @@ defmodule Mix.Tasks.HelpTest do
end
end

test "help list alias task", context do

This comment has been minimized.

@fertapric

fertapric Jun 27, 2018
Member

Minor nitpick: I think it should be "lists", and maybe "help lists all aliases"?


in_tmp(context.test, fn ->
Mix.Tasks.Help.run(["--search", "c"])
assert_received {:mix_shell, :info, ["mix c" <> _]}

This comment has been minimized.

@fertapric

fertapric Jun 27, 2018
Member

Could this test give false positives due to the match of other tasks like mix compile?

This comment has been minimized.

## Arguments
mix help - prints all tasks and their shortdoc
mix help - prints all aliases, tasks and task shortdoc
mix help ALIAS - prints the definition for the given alias
mix help TASK - prints full docs for the given task
mix help --search PATTERN - prints all tasks that contain PATTERN in the name

This comment has been minimized.

@fertapric

fertapric Jun 27, 2018
Member

"prints all tasks and aliases that contain PATTERN in the name"?

This comment has been minimized.

@Sihui

Sihui Jun 27, 2018
Author Contributor

good catch!

Sihui added 3 commits Jun 27, 2018

in_tmp(context.test, fn ->
Mix.Tasks.Help.run(["--search", "h"])
assert_received {:mix_shell, :info, ["mix h" <> _]}

This comment has been minimized.

@fertapric

fertapric Jun 27, 2018
Member

Unfortunately, this might also give false positives with mix help itself, mix hex or any mix hex.*.

This comment has been minimized.

@fertapric

fertapric Jun 27, 2018
Member

I would either include a space, or match on the message live above:

assert_received {:mix_shell, :info, ["mix h" <> message]}
assert message =~ ~r/# Alias defined in mix.exs/
Sihui added 3 commits Jun 27, 2018
@smorin
Copy link
Contributor

@smorin smorin commented Jun 27, 2018

@josevalim makes sense moving to core-mailing list.

I would say that ad-hoc aliases would end-up being more clutter than well-structured tasks but I see your point. We should move the discussion about revamping aliases elsewhere then. Maybe a proposal to the elixir-lang-core mailing list as the scope of this particular issue is well defined based on how aliases work today and we should not go off-topic.

@Sihui
Copy link
Contributor Author

@Sihui Sihui commented Jun 30, 2018

@josevalim @michalmuskala @whatyouhide Can this be merged? Anything else I need to do?

@josevalim
Copy link
Member

@josevalim josevalim commented Jun 30, 2018

|> Enum.map(fn {k, _} -> Atom.to_string(k) end)
Enum.map(Mix.Project.config()[:aliases], fn {alias_name, _} ->
Atom.to_string(alias_name)
end)

This comment has been minimized.

@whatyouhide

whatyouhide Jun 30, 2018
Member

Why not use aliases = load_aliases() here too?

This comment has been minimized.

@whatyouhide

whatyouhide Jun 30, 2018
Member

I mean that and then Enum.sort(Map.to_list(aliases) ++ tasks).

This comment has been minimized.

@Sihui

Sihui Jun 30, 2018
Author Contributor

Map.to_list(aliases) won't work because aliases is [{alias1, taks1}, {alias2, task2}, ...].
If we want to use load_aliases(), we might need to do

    aliases = Enum.map(load_aliases(), fn {alias_name, _} -> alias_name end)

    for info <- Enum.sort(aliases ++ tasks) do
      Mix.shell().info(info)
    end

At that point, we might as well use Mix.Project.config()[:aliases] directly, so we don't have to iterate twice.

This comment has been minimized.

@whatyouhide

whatyouhide Jun 30, 2018
Member

Oh I missed that it was just the alias name, sorry!

@whatyouhide whatyouhide merged commit 6b5cb70 into elixir-lang:master Jun 30, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@whatyouhide
Copy link
Member

@whatyouhide whatyouhide commented Jun 30, 2018

Thanks @Sihui! 💟

@Sihui
Copy link
Contributor Author

@Sihui Sihui commented Jun 30, 2018

Thank you all for reviewing! ❤️ ❤️ ❤️

@Sihui Sihui deleted the Sihui:mix-help-list-alias branch Jun 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants