search

duckduckgo / p5-app-duckpan

DuckDuckHack OpenSource Development Application
http://metacpan.org/module/App::DuckPAN
Other
53 stars 47 forks source link

DuckPAN should show an error message when Cheat Sheet JSON fails to parse correctly #293

Closed moollaza closed 8 years ago

moollaza commented 8 years ago

Currently DuckPAN throws an error but continues execution. The Cheat Sheet with broken JSON will not be able to trigger though which isn't very clear.

We should probably throw a (fatal?) error with a descriptive message that explain which file has broken JSON and that it will not work until the JSON is fixed.

We could even suggest they use JSONLint or similar to find and solve the problem

moollaza commented 8 years ago

We may actually be able to handle this here: https://github.com/duckduckgo/zeroclickinfo-goodies/blob/master/lib/DDG/Goodie/CheatSheets.pm#L37

I'm not entirely sure yet...

sd43 commented 8 years ago

@moollaza I'd like to take a shot at this if no one else is working on this currently. Is that alright?

moollaza commented 8 years ago

It's all yours!

sd43 commented 8 years ago

@moollaza Sorry I couldn't work on this for a while. I have written some code to fix the issue, but before I send the PRs, could you please take a look at the following commits and give me some feedback if the idea is acceptable?

The idea is to introduce a dev_essential function in DDG::Meta::Information in this commit. This function can then be called in IAs to say that they are "essential for IA developers and failure to load may cause issues for them". For example, I've made the change in the CheatSheets.pm file in this commit. Then, when duckpan server or duckpan query is executed, they terminate early if an 'essential' IA fails to load. This change has been implemented in this commit.

moollaza commented 8 years ago

@srvsh apologies for the late reply.

Meta::Information will be removed soon as the IA Pages have taken ownership of the data.

With respect to dev_essential I'm not sure what would be considered essential and non-essential, but for the most part devs usually only care about their own IA. Right now when modules fails to load, DuckPAN collects their names and let's you know which modules didn't load. If they have missing dependencies it also tells you that. This was implemented to gracefully handle missing Perl deps.

I think cheat sheets should follow suit and maybe go a little further to point out which file is causing problems. As long as it's really clear to developers I think its okay.

As an example, I've broken a cheat sheet and a regular Goodie and the error message for the Cheat Sheet is much more vague:

1_____ssh_

Does this make sense?

sd43 commented 8 years ago

@moollaza Thanks for reviewing the implementation and giving your feedback! I haven't been getting much time for open source work recently, so I think it's best that I don't hold on to this issue any longer and let someone else take over. I'll write down my intention with the implementation more in detail below with hope that it may be of help.

The intention was that instant answers that are developed internally at DuckDuckGo with the aim of being used by IA developers - such as CheatSheets - would be marked as 'dev_essential'. I'm using Meta::Information to add the dev_essential subroutine so that IAs can call it to mark themselves as important. These essential IAs should fail fast and die if they detect that there's a problem - for example, the CheatSheets IA should die if a JSON file is not valid.

The DuckPAN app (specifically server and query) would then specially handle failures of essential IAs and inform the user that there was a problem. The message shown here is the message that the IA died with. In my implementation, the user is informed and then app exits to ensure the user gets the message. I do feel that your suggestion to show the messages and let the app to continue running is a better choice though.

Thanks!

GuiltyDolphin commented 8 years ago

@moollaza I've fixed up some of the cheat sheet tests (again) in duckduckgo/zeroclickinfo-goodies#2740 - there is now correct reporting of invalid JSON. Is this satisfactory? As an extra step we could force duckpan to test IAs before running query or server to report errors, but hopefully just using duckpan test (my_cheat_sheet) should be satisfactory (added in #317).

moollaza commented 8 years ago

there is now correct reporting of invalid JSON. Is this satisfactory?

Sounds good to me!

As an extra step we could force duckpan to test IAs before running query or server to report errors, but hopefully just using duckpan test (my_cheat_sheet) should be satisfactory (added in #317)

DuckPAN does load all the PM modules, so before cheat sheets, that was technically the case. Anything that fails to load generates a warning (usually informs you the Perl has a syntax error or missing dependency) but this doesn't happen with Cheat Sheets.

I think we should rely on the tests because loading all the JSON would delay the startup time for the server, which we took steps to reduce a few years back