Skip to content
This repository has been archived by the owner on Oct 15, 2022. It is now read-only.

DuckPAN/DDG: DuckPAN won't die when a module fails to load. #78

Merged
merged 12 commits into from Jun 19, 2014

Conversation

jagtalon
Copy link
Member

It's been a perennial problem for DuckPAN users to install all of the dependencies of each IA in the repo before they can even test anything. This patch prevents DuckPAN from exiting when it fails to load an IA. It goes on running, but it warns the user that some IAs didn't load (along with the errors that caused it). This is especially useful if the dependency is not only a Perl module (which is generally easy to install), but external libraries that may have different installation methods depending on the system (I'm looking at you GNU MPFR which needs sudo apt-get install libmpfr-dev on Ubuntu or brew install mpfr on OS X).

Here it is in action (ChineseZodiac missing a module):
screen shot 2014-06-18 at 12 15 49 pm

Here it is displaying some other error:
screen shot 2014-06-18 at 12 16 24 pm

CC @moollaza @nilnilnil

jagtalon added 3 commits June 18, 2014 15:53
It's been a perennial problem for DuckPAN users to install all of the dependencies of each IA in the repo before they can even test anything. This patch prevents DuckPAN from exiting when it fails to load an IA. It goes on running, but it warns the user that some IAs didn't load (along with the errors that caused it).
@nilnilnil
Copy link
Contributor

I'm cool with this as per our discussion yesterday. I'd like @moollaza to sign off. Also, from what I can tell, if a module requires more than one unmet dependency the user will have to keep reloading?

@jagtalon
Copy link
Member Author

@nilnilnil That is the case--I don't know how to get all the missing dependencies at the moment. I'll make an issue for that.

@hunterlang
Copy link

an easy way to fix that is to just do dzil listdeps --missing and display the output in the duckpan output. it's not ideal because then duckpan won't display the full mapping of missing dependencies -> unloaded IAs, but at least the user will get some sense of everything that's missing. drawbacks could be that this output is huge and/or the user doesn't actually want everything and just wants to know what to install for a certain goodie. then we'd need a bit more code to associate each dependency with the corresponding IA (something a grep could solve)

@nilnilnil
Copy link
Contributor

@moollaza
Copy link
Member

I was going to recommend something similar to what @nilnilnil has there. If the class fails to load, the error might as well list out all the dependencies of the class (I think it should be a relatively small list?) and then that way they can quickly install all the deps.

Maybe the error message should also let them know they can install all deps by running duckpan installdeps, but let them know it might take a while.

Also @jagtalon please fix the indentation (DuckPAN code uses tabs).

sub print_failed_modules {
my %failed_to_load = %{shift @_};

print "\nSome instant answers failed to load:\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

how about less fail-oriented: "These instant answers were not loaded"

@russellholt
Copy link
Contributor

Looks good

@nilnilnil
Copy link
Contributor

Alternatively, for modules failing the dependency check at all, you could modify the output of the error message to be something like:

Module $spice has unmet dependencies, if you plan to work on them, run grep 'use' $spice | grep -v 'DDG::' | awk {'print $2'} | sed s/\;$// | cpanm to satisfy them.

@jagtalon
Copy link
Member Author

So I just added some online documentation if they need any help: duckduckgo/duckduckgo-documentation#81

screen shot 2014-06-18 at 4 13 32 pm

@jagtalon
Copy link
Member Author

@moollaza Should be ready. :) Sorry for the mess--shouldn't have converted everything to spaces

@moollaza
Copy link
Member

@jagtalon I preferred the previous approach because the message was way more helpful. At the very least it gave the name of a dependency that I could quickly install.

Even if I follow the link it just tells me how that I need to use cpanm to install a dependency, which means I need to go back to the file, open it, and look at it.

I think the error message should be similar to how you first had it and it should tell you to check the file for other dependencies. We should still suggest you read that article though if you need more info on installing Perl deps.

Something like this seems good:

The following instant answers were not loaded:
{
    DDG::Goodie::ChineseZodiac - "Missing dependencies! Please install <module> and any other required dependencies to use this instant answer. Check lib/DDG/ChineseZodiac.pm to see what other dependencies are required."
    ...
}

To learn more about installing Perl dependencies, please read http://duck.co/duckduckhack....
Note: If you are not working on these Instant Answers you can ignore these messages.

@jagtalon
Copy link
Member Author

@moollaza Added the change to the message--that's clearer. I didn't add the Missing dependencies! though (too alarming) or Check lib/DDG/ChineseZodiac.pm to see what other dependencies are required. (it's a bit too long and it's explained in the link anyway):

screen shot 2014-06-18 at 6 40 29 pm

@moollaza
Copy link
Member

LGTM 👍

moollaza added a commit that referenced this pull request Jun 19, 2014
DuckPAN/DDG: DuckPAN won't die when a module fails to load.
@moollaza moollaza merged commit d7e65eb into master Jun 19, 2014
@moollaza moollaza deleted the jag/loading-error branch June 19, 2014 00:44
@jagtalon
Copy link
Member Author

YES!

@russellholt
Copy link
Contributor

FWIW I think the language here, "Note:.. " is too passive.

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

Successfully merging this pull request may close these issues.

None yet

5 participants