search

duckduckgo / p5-app-duckpan

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

Triggers: Warn developer if trigger has uppercase letters #245

Closed moollaza closed 9 years ago

moollaza commented 9 years ago

We auto lowercase the query when comparing it against the trigger. We don't actually bother lowercasing the trigger (we could but it won't make much difference, and casing can always be verified in the handle) but we don't warn the user when their trigger has uppercase letters, which means they're creating a trigger that will never match.

DuckPAN should be able to detect this and throw a fatal error (or a really noticeable warning).

Arguably this could just be a change to the https://github.comduckduckgoduckduckgo.git repo.

/cc @zachthompson @jagtalon any thoughts on this?

zachthompson commented 9 years ago

This almost seems more like a development QC step than something we should put in the code. Wouldn't a developer know that their IA isn't triggering? I would think between the dev and reviewer this would be caught. It could be noted in the Triggers Overview as well (we might want to remove the mixed case examples?)

I think it would need be somewhere around here https://github.com/duckduckgo/duckduckgo/blob/master/lib/DDG/Block/Blockable/Triggers.pm#L69. So I'm not sure we can limit it to duckpan. Is there somewhere else you were thinking we could check these?

moollaza commented 9 years ago

@zachthompson I agree that it's a QC step, but I think seeing as this is a tool for developers we might as well help them as much as we can by raising flags when we detect bugs (like alerting on the frontend about the Spice.failed() calls), especially stuff like this which is something very subtle and hard to easily determine (especially for first time devs), because it's not very intuitive in this case why the IA isn't triggering. Spent a little time last night working with a new dev who couldn't figure this out and it took me a bit to realize it as well.

Similar pernicious bugs would be when the API returns raw JSON (instead of JSONP) -- its usually not something you quickly pick up on (i.e. it took me a while to notice that during the Spice Training talk :disappointed: )

moollaza commented 9 years ago

It could be noted in the Triggers Overview as well (we might want to remove the mixed case examples?)

Totally agree on that, we should definitely document it well, but I think that we should be doing a lot more error checking than we currently do to make it easier for devs.

zachthompson commented 9 years ago

There's probably a place we can call get_triggers_of_plugin (or similar) and review them from duckpan without changing anything in DDG::*. I'm all for having as many QC checks as possible during dev.

zachthompson commented 9 years ago

Perhaps this should be a test case in each repo as well. Do we have a test that isn't IA-specific in which we could add these?

moollaza commented 9 years ago

There's probably a place we can call get_triggers_of_plugin @zachthompson absolutely! We can certainly check the triggers for a given IA. https://github.com/duckduckgo/duckduckgo/blob/master/t/40-goodie.t#L17

I'm not sure about the best time to do this check -- I think it would have to be when we load the IAs/Block?

If we added the check to DDG::* I suppose that would happen automatically (like how the Perl metadata croaks when it's invalid)

Perhaps this should be a test case in each repo as well. Do we have a test that isn't IA-specific in which we could add these?

Do you mean like a dzil test that would check all the IA's in the repo? That's interesting, I could see something like that working. Or perhaps we could add it to the Test::Spice fn or similar so when it loads it for testing it verifies it's good for testing?