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

add quick search to hexdocs.pm #574

Merged
merged 8 commits into from
Jan 23, 2022
Merged

add quick search to hexdocs.pm #574

merged 8 commits into from
Jan 23, 2022

Conversation

gofenix
Copy link
Contributor

@gofenix gofenix commented Jul 17, 2021

Change-Id: Ifd888136b0c7cd9936bf02612bd87fdcce639ba7

this pr is a hover enhancement.

before
image

and after this pr
image

we can click this hyperlink to hexdocs.pm search page, like this: https://hexdocs.pm/elixir/search.html?q=File.write
image

Change-Id: Ifd888136b0c7cd9936bf02612bd87fdcce639ba7
@axelson
Copy link
Member

axelson commented Jul 17, 2021

Hi! Thanks for this. I think this is a very interesting idea but there a few issues that need to be resolved:

Change-Id: I2f858ccab0a13fac426666263d59f4cf215db8a4
@gofenix
Copy link
Contributor Author

gofenix commented Jul 19, 2021

Thanks for your reply.

according to you suggestions, I made some optimizations.

for issue1, only elixir std modules will be add hexdocs link. for example:

std module:
image

other module:
image

and for issue2, now can jump to function location.
image

mod_name = module_name(t)

if elixir_module?(mod_name) do
"https://hexdocs.pm/elixir/#{module_name(subject)}.html##{func_name(subject)}/#{params_cnt(t)}"

Choose a reason for hiding this comment

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

its better to place the base hexdocs url into a module variable

func?(t) ->
mod_name = module_name(t)

if elixir_module?(mod_name) do

Choose a reason for hiding this comment

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

I think we can go even further.
With mod_name.__info__(:compile) we can determine wether the module is from a dependency or not. Therefore, generate hexdocs for modules from dependencies as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i have fixed it.

s =~ ~r/.*\..*\(.*\)/
end

defp elixir_module?(s) do

Choose a reason for hiding this comment

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

Change-Id: Ief6e0fd0c13b2e42ddaf1817dfbf20b600971011
@gofenix
Copy link
Contributor Author

gofenix commented Jul 21, 2021

@mpanarin thanks for your review.

I have made some changes according to your suggestion, now it can jump to both third party dependences and elixir std modules.

eg.
image

@lukaszsamson
Copy link
Collaborator

lukaszsamson commented Jul 25, 2021

@zhenfeng-zhu please add some tests (for builtin, external, umbrella, erlang modules). Please also fix build on elixir 1.12

zhuzhenfeng.code added 3 commits July 27, 2021 09:55
Change-Id: I25228974078c7f09c6afc748c22144556b5cdc63
Change-Id: I7e6debee937d2e4de5a3b990e4086b59e78a3aba
Change-Id: Iff48c16cf6a5d518004f7f211235e0be085985f2
@lukaszsamson
Copy link
Collaborator

Now we need some tests and IMO it's good to go

@gofenix
Copy link
Contributor Author

gofenix commented Jul 29, 2021

Now we need some tests and IMO it's good to go

yeah, I am doing. Thanks for your help.

Change-Id: Id40baed74aeecf62f086f158185ed03345ef5ee0
@gofenix
Copy link
Contributor Author

gofenix commented Jul 31, 2021

Now we need some tests and IMO it's good to go

@lukaszsamson I have add some tests for hover.But I don’t think it’s perfect yet. Could you give more advice on writing tests?

@mpanarin
Copy link

mpanarin commented Sep 9, 2021

@lukaszsamson can this be looked at please? This feature is pretty good and can be built upon

@gofenix
Copy link
Contributor Author

gofenix commented Sep 14, 2021

@mpanarin any update?

@gofenix
Copy link
Contributor Author

gofenix commented Sep 25, 2021

@lukaszsamson @mpanarin @axelson Hope you anyone could review it

@lukaszsamson
Copy link
Collaborator

I asked for tests of erlang module and function (e.g. :ets :ets.init should link to erlang docs, but :telemetry should link to hexdocs).
I also need a test of umbrella app to see if project dir is correctly handled.

@gofenix
Copy link
Contributor Author

gofenix commented Sep 26, 2021

I asked for tests of erlang module and function (e.g. :ets :ets.init should link to erlang docs, but :telemetry should link to hexdocs).
I also need a test of umbrella app to see if project dir is correctly handled.

thanks, i'm trying to do it!

@gofenix
Copy link
Contributor Author

gofenix commented Sep 26, 2021

I asked for tests of erlang module and function (e.g. :ets :ets.init should link to erlang docs, but :telemetry should link to hexdocs).

For this issue, I have a question:

I find that both of :ets and : telemetry are erlang modules. Why :ets go to erlang docs , but :telemetry should go to hexdocs?

That make me confused, what is the standard of judgment?

@lukaszsamson
Copy link
Collaborator

You should ask OTP team why they don’t publish docs to hexdocs 😉

@gofenix
Copy link
Contributor Author

gofenix commented Sep 27, 2021

You should ask OTP team why they don’t publish docs to hexdocs 😉

Haha, maybe in this pr, I can just not support erlang modlue link to hexdocs.

And by the way, in my test file:

def fake_dir() do
    Path.join(__DIR__, "../../../..") |> Path.expand() |> maybe_convert_path_separators()
end

I use fake_dir() as a argument as project_dir.

the fake_dir is "/Users/lucas/Documents/demos/elixirs/vscode-elixir-ls/elixir-ls", it's a umbrella app. so I guess the work is done

Change-Id: I77d984c72513b75d84958bdc63db093b7028142f
@lukaszsamson lukaszsamson merged commit 2e7ac6d into elixir-lsp:master Jan 23, 2022
@lud
Copy link

lud commented Feb 9, 2022

Hello,

With this change, ElixirLS does not work anymore on our projects, because we are working on two projects called like SomeCustomer.SomeSubproject and SomeCustomer.DataSchemas.

As there is no SomeCustomer root module, we do not have any hover information for any of our modules.

It looks like adding a simple rescue clause solves the problem:

  defp hexdocs_link(hd, subject, project_dir) do
    # ...
  rescue
    _ in UndefinedFunctionError -> ""
  end

@lukaszsamson
Copy link
Collaborator

lukaszsamson commented Feb 9, 2022

@lud please open a bug report

Edit: already tracked in #673

@lud
Copy link

lud commented Feb 12, 2022

Hi sorry I had to leave my home station quickly. Happy to see that is already solved.

Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants