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

Modify hex docs.open to fetch docs if not present locally #293

Merged
merged 1 commit into from
Sep 24, 2016

Conversation

fteem
Copy link
Contributor

@fteem fteem commented Sep 24, 2016

Took a stab at #290

With this implementation, the task checks if the documentation for the package is locally present. Otherwise, it will fetch the latest version and open it in the browser.

Copy link
Member

@ericmj ericmj left a comment

Choose a reason for hiding this comment

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

Can you investigate why the tests fail?

end
unless File.exists?(doc_path) do
fetch_docs([name, latest_version], opts)
end
open_docs([name, latest_version], opts)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be open_file like below? It looks like this would loop infinitely.

Copy link
Contributor

@whatyouhide whatyouhide Sep 24, 2016

Choose a reason for hiding this comment

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

I guess it should be open_docs_offline([name, latest_version], opts) so we can drop the unless File.exists? from here as well.

@@ -123,6 +126,14 @@ defmodule Mix.Tasks.Hex.Docs do
open_file(index_path)
end

defp find_package_version(name, opts) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I am not very happy with this function, but we somehow need check the latest version based on cache or remote version.

Copy link
Member

Choose a reason for hiding this comment

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

It looks good imo.

@@ -113,7 +113,10 @@ defmodule Mix.Tasks.Hex.Docs do
end

defp open_docs_offline([name], opts) do
latest_version = find_latest_version("#{opts[:home]}/#{name}")
{ is_missing, latest_version } = find_package_version(name, opts)
if is_missing && Mix.shell.yes?("Documentation file not found, would you like to download it?") do
Copy link
Member

Choose a reason for hiding this comment

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

This should be done automatically, we should not prompt the user. If the user says no, it would just crash anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ericmj it was discussed in #290 that we should prompt the user, that's why I added it.

@@ -113,7 +113,10 @@ defmodule Mix.Tasks.Hex.Docs do
end

defp open_docs_offline([name], opts) do
latest_version = find_latest_version("#{opts[:home]}/#{name}")
{ is_missing, latest_version } = find_package_version(name, opts)
Copy link
Member

Choose a reason for hiding this comment

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

Remove the spaces inside {}.

@ericmj
Copy link
Member

ericmj commented Sep 24, 2016

Sorry for the back and forth but we should not prompt the user in this
case. I don't see the benefit of it.

On Sat, Sep 24, 2016 at 8:15 PM, Ilija Eftimov [email protected]
wrote:

@fteem commented on this pull request.

In lib/mix/tasks/hex/docs.ex #293:

@@ -113,7 +113,10 @@ defmodule Mix.Tasks.Hex.Docs do
end

defp open_docs_offline([name], opts) do

  • latest_version = find_latest_version("#{opts[:home]}/#{name}")
  • { is_missing, latest_version } = find_package_version(name, opts)
  • if is_missing && Mix.shell.yes?("Documentation file not found, would you like to download it?") do

@ericmj https://github.com/ericmj it was discussed in #290
#290 that we should prompt the user,
that's why I added it.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#293, or mute the thread
https://github.com/notifications/unsubscribe-auth/AATV2r5yJA3CZZP5ZcDMyIz06eUG4nSPks5qtWjSgaJpZM4KFmpx
.

Eric Meadows-Jönsson

@fteem
Copy link
Contributor Author

fteem commented Sep 24, 2016

No probs. PR updated.

Copy link
Member

@ericmj ericmj left a comment

Choose a reason for hiding this comment

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

Thank you!

@ericmj ericmj merged commit c89dda8 into hexpm:master Sep 24, 2016
@fteem fteem deleted the docs_open_fetch branch September 24, 2016 18:39
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.

3 participants