Skip to content

Conversation

wojtekmach
Copy link
Member

@wojtekmach wojtekmach commented Oct 25, 2018

Closes #910

screenshot 2018-10-25 at 14 03 53

}) == "[`mix hex.publish`](#{@elixir_docs}hex/Mix.Tasks.Hex.Publish.html)"
end

test "does not autolink task with arguments" do
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as the 1st step I skipped it but autolinking these too can be nice, see screenshot.

@fertapric
Copy link
Member

I would update the documentation here, and maybe handle :mix_task here?

@wojtekmach
Copy link
Member Author

I don't think I need to extend that function because I'm just adding {:mix_task, :elixir, :normal} and not also {:mix_task, :erlang, :normal} pass; as far as I understand, we need Erlang vs Elixir pass because modules look different whereas here it's always the same: mix TASK. If it's gonna make code more consistent though I'll handle that; @eksperimental?

I'll update the type spec, good catch!


defp re_kind_language(:mix_task, :elixir) do
~r{
mix\ ([a-z]+[a-z0-9\._]*)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we handle more than one white-space?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep it as is, if someone accidentally adds more it won't highlight so it's gonna be easier to spot.

Copy link
Contributor

@eksperimental eksperimental Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is that if a test editor wraps the line and we end up with

`mix
task argument`

or

`mix task
arguments`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eksperimental i think we can wait until it happens.

Copy link
Contributor

@eksperimental eksperimental Nov 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fertapric
Copy link
Member

@wojtekmach I would also update the code comment:

kind is either :function or :module.

@wojtekmach wojtekmach mentioned this pull request Oct 25, 2018
kind <- [:module, :function] do
{kind, language, link_type}
end)
end) ++ [{:mix_task, :elixir, :normal}]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could also support :custom links.
such as we do with the rest of the funtions.

assert project_doc("[the :erlang.apply/2 function](`Kernel.apply/2`)", %{}) ==
"[the :erlang.apply/2 function](#{@elixir_docs}elixir/Kernel.html#apply/2)"

      assert project_doc("[the :erlang.apply/2 function](`Kernel.apply/2`)", %{}) ==
               "[the :erlang.apply/2 function](#{@elixir_docs}elixir/Kernel.html#apply/2)"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fertapric convinced me that custom links can sometimes hurt IEx experience so how about we hold off on this change for now?

Copy link
Contributor

@eksperimental eksperimental Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fertapric convinced me that custom links can sometimes hurt IEx experience so how about we hold off on this change for now?

Is that something that we could improve on the IEx side?

# `language` can be: `:elixir`, `:erlang` or `:markdown`.
#
# `kind` is either `:function` or `:module`.
# `kind` is either `:function`, `:module`, or `:mix_task`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with :mix_task, but I'm wondering. Is there any other kind of task?
Should it be called just task?
is there a equivalent of a Mix Task in Erlang?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are Rebar tasks which we might similarly support one day but not sure how hard that would be.

Copy link
Contributor

@eksperimental eksperimental Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beacuse I'm seeing we are calling {:mix_task, :elixir, :normal}, it is a bit redundant I guess.
{:mix_task, :elixir, :normal}, and in the future we could call {:task, :erlang, :normal} looks better IMO

My only concern is that there are other tools other than Mix and Rebar for creating tasks that we may support

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for mix_task because that's also what we call it in the sidebar.

@eksperimental
Copy link
Contributor

@wojtekmach: It looks awesome at first glance, I would like to give it a try, tinker with it, and get back to you with more feedback if needed, but unfortunately I'll have to do this later.

@wojtekmach
Copy link
Member Author

@eksperimental great, no rush! Also just wanted to say that it was very easy to add this functionality after your refactoring, thanks for that.

@eksperimental
Copy link
Contributor

eksperimental commented Oct 25, 2018

@eksperimental great, no rush! Also just wanted to say that it was very easy to add this functionality after your refactoring, thanks for that.

@wojtekmach you are welcome! The refactoring came as a must do, after I spent so long trying to fix a bug, and I wasn't able to.
As I was triyng to fix the bug, I wondered how you manged to get some those fixed before and implement new features :-D

@wojtekmach
Copy link
Member Author

easy: fix 1 bug, add 2 more, rinse and repeat, wait for a hero to clean up 😂

Co-Authored-By: wojtekmach <wojtek@wojtekmach.pl>
Copy link
Contributor

@eksperimental eksperimental left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only three minor corrections.

end
end

defp replace_fun(:mix_task, :elixir, _link_type, options) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`since we are only supporting :normal links, this line can read:

defp replace_fun(:mix_task, :elixir, :normal, options) do


defp re_kind_language(:mix_task, :elixir) do
~r{
mix\ ([a-z]+[a-z0-9\._]*)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the + is redundant in the regex

end
end

describe "mix tasks" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mix tasks

@wojtekmach wojtekmach merged commit da6b625 into elixir-lang:master Oct 27, 2018
@wojtekmach wojtekmach deleted the wm-autolink-mix-tasks branch October 27, 2018 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants