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

Bible : Show Random Verse #2826

Closed

Conversation

rajeshwarisah
Copy link

Fixes #2820 - adds trigger to random and shows random verses
#2820
https://duck.co/ia/view/bible
@hunterlang

@daxtheduck
Copy link

@MAMAMIA9 Hey!

Thanks for taking the time to contribute! We really appreciate it.

We work closely with every contributor to make Instant Answers the best they can be, so we appreciate your patience as we look over your code. From here, the process usually goes like this:

  1. Pull Request is reviewed by the DuckDuckGo staff and community
  2. Staff and community will leave feedback with any necessary updates to the function or design.
  3. Once you've made any necessary corrections, then your Instant Answer will be merged and deployed live on DuckDuckGo!

If you have any questions along the way, feel free to ask them here. Our staff and community are also available on Slack to answer any questions you may have. If you'd like to join us there please head to https://quackslack.herokuapp.com/ to get an invite.

More Info: http://docs.duckduckhack.com/submitting/submitting-overview.html

Thanks!

@daxtheduck
Copy link

daxtheduck commented Jun 16, 2016

Bible

Description: Provides the text of selected Bible verses from the NET Bible translation.

Example Query: [genesis 15:7](https://beta.duckduckgo.com/?q=genesis 15:7), [bible genesis 26:4](https://beta.duckduckgo.com/?q=bible genesis 26:4)

Tab Name: Answer

Source:

These are the important fields from the IA page. Please check these for errors or missing information and update the IA page


This is an automated message which will be updated as changes are made to the IA page

@@ -12,6 +12,9 @@ spice to => 'http://labs.bible.org/api/?type=json&callback={{callback}}&formatti
handle query_lc => sub {
if ($_ =~ /^bible\s+([a-z]+\s*?[0-9]+:[0-9]+)$/i || $_ =~ /^((?:genesis|exodus|leviticus|numbers|deuteronomy|joshua|judges|ruth|1 samuel|2 samuel|1 kings|2 kings|1 chronicles|2 chronicles|ezra|nehemiah|esther|job|psalm|proverbs|ecclesiastes|song of solomon|isaiah|jeremiah|lamentations|ezekiel|daniel|hosea|joel|amos|obadiah|jonah|micah|nahum|habakkuk|zephaniah|haggai|zechariah|malachi|matthew|mark|luke|john|acts|romans|1 corinthians|2 corinthians|galatians|ephesians|philippians|colossians|1 thessalonians|2 thessalonians|1 timothy|2 timothy|titus|philemon|hebrews|james|1 peter|2 peter|1 john|2 john|3 john|jude|revelation)\s+[0-9]+:[0-9]+)$/) {
return $1 if $1;
}
if ($_ =~ /^bible\s+(?:random|random verse)$/i){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please could you put a space between ) and {

@pnpninja
Copy link
Collaborator

@tagawa : is it necessary that @hunterlang give some input on this?

@tagawa
Copy link
Collaborator

tagawa commented Jun 16, 2016

@pnpninja Yes, I'm going to deploy to beta soon (there's currently a separate install in progress) and then we can test it - both the maintainer and community.

@pnpninja
Copy link
Collaborator

@tagawa : sweet! thanks bro

@daxtheduck daxtheduck deployed to beta.duckduckgo.com June 16, 2016 13:17 Active
@tagawa
Copy link
Collaborator

tagawa commented Jun 16, 2016

It's here on the beta server now: https://beta.duckduckgo.com/?q=bible+random+verse&ia=answer
Good, quick work @MAMAMIA9!

One thing I've noticed is that it would be good if it also appeared for random bible verse. Do you think it's possible to add that?

@tagawa
Copy link
Collaborator

tagawa commented Jun 16, 2016

Actually, maybe even random verse bible and random verse from the bible, but maybe that would be pushing the regex too far :-)

@pnpninja
Copy link
Collaborator

@tagawa : we'll see what we can do...

Added additional triggers
More triggers
Added additional tests for additional triggers that are getting accepted
@pnpninja
Copy link
Collaborator

@tagawa : added additional triggers and the tests...

@daxtheduck daxtheduck deployed to beta.duckduckgo.com June 17, 2016 07:03 Active
@pnpninja
Copy link
Collaborator

@tagawa : looking all good in the Beta 🤘🏼

@tagawa
Copy link
Collaborator

tagawa commented Jun 17, 2016

Yay! random bible verse seems to be a popular query so this is a really nice improvement.
Let's leave it on beta for the community to test before we merge this.

@tagawa
Copy link
Collaborator

tagawa commented Jun 17, 2016

One thing I've just noticed - with the query random bible verse I'm getting the About tab opening, and I have to click on the Answer tab to make this IA visible. To increase the priority of this IA, please could you add this line to the meta object within Spice.add in the share/spice/bible/bible.js file?

signal: 'high'

Set signal to high to prevent some Fathead from triggering on `random bible verse` query
@pnpninja
Copy link
Collaborator

@tagawa : oh yea - i saw it... I think i've fixed it ...

@daxtheduck daxtheduck deployed to beta.duckduckgo.com June 17, 2016 09:50 Active
@@ -17,7 +17,8 @@
meta: {
sourceName: 'Bible',
sourceUrl: source_url,
sourceIconUrl: 'http://bible.org/sites/bible.org/files/borg6_favicon.ico'
sourceIconUrl: 'http://bible.org/sites/bible.org/files/borg6_favicon.ico',
signal: 'high'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, you were right first time. This should be above the meta object, next to id, name, etc. Sorry!

@daxtheduck daxtheduck deployed to beta.duckduckgo.com June 17, 2016 11:29 Active
@tagawa
Copy link
Collaborator

tagawa commented Jun 17, 2016

Thanks @pnpninja - this works for me now. How about you?

@pnpninja
Copy link
Collaborator

@tagawa - yup ! it works

@tagawa
Copy link
Collaborator

tagawa commented Jun 17, 2016

@MAMAMIA9 @pnpninja: @zekiel noticed that we're caching the API result (it's on by default for one hour). Did you have trouble with their API limits when you turned it off during testing? If so, could we try adding a line in Bible.pm to cache for, say 5 minutes, e.g.

spice proxy_cache_valid => "200 5m";

More info about caching: http://docs.duckduckhack.com/backend-reference/api-reference.html

@pnpninja
Copy link
Collaborator

pnpninja commented Jun 17, 2016

@tagawa - yes. sometimes, we get a 500 error.. I think its best we cache for 5 min.. I'll do it

@gaulrobe
Copy link
Collaborator

@MAMAMIA9 @pnpninja @tagawa Any kind of caching when there is a "random" query might cause user frustration right? Ideally, we don't want to cache those so that people have a true, random experience?

@tagawa
Copy link
Collaborator

tagawa commented Jun 17, 2016

@gaulrobe We'd likely get a 500 error from the API if there's no caching, but then the user wouldn't see the error - just no IA would appear. So it's a choice between that or possible repeated IA content. What do we think is best?

@gaulrobe
Copy link
Collaborator

@tagawa could we refactor this IA to be two separate modules/tests? One for random verses, which isn't cached and the other for specific bible verses which can probably be cached for a long time. Is that something we can do here?

@tagawa
Copy link
Collaborator

tagawa commented Jun 17, 2016

@moollaza Do you know if what @gaulrobe is suggesting is possible within a single IA?

@tagawa
Copy link
Collaborator

tagawa commented Jun 20, 2016

@gaulrobe True.

@moollaza What do you reckon? I think we have three options:

  1. Don't cache, and accept that the IA won't appear sometimes, both for random and non-random queries.
  2. Cache, and accept that people will see a repeated result if they repeat the search.
  3. Split the IA into two - one which caches and one which doesn't.

@pnpninja
Copy link
Collaborator

ping @moollaza

@@ -14,6 +14,7 @@
id: 'bible',
name: 'Answer',
data: result,
signal: 'high',
Copy link
Member

Choose a reason for hiding this comment

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

We should first try to handle this with the Block Group on the IA Page.

@tagawa recently @bsstoner told me we should't set the signal "high" like this because it can confuse the AnswerBar JS.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, OK. So for IAs that aren't appearing as the default tab but should be, we should set it internally instead?

Copy link
Member

Choose a reason for hiding this comment

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

Yup 👍

@moollaza
Copy link
Member

@moollaza What do you reckon? I think we have three options:

I don't understand why we're seeing any 500 responses? That seems like a bigger problem.

Given the nature of the "Random Verse" IA -- I think it's best to make this into a separate IA. We shouldn't sacrifice the speed and caching of the existing IA to support the random Bible verses results. The alternative, a Random IA that is only random every 5 minutes will probably give the impression that it's broken.

We will need to namespace them accordingly: e.g. DDG::Spice::Bible::Lookup and DDG::Spice::Bible::Random. That will need to be two separate PRs. One for updating the current Spice, another adding the new one.

@tagawa
Copy link
Collaborator

tagawa commented Jun 22, 2016

@MAMAMIA9 @pnpninja Sorry but could we make this into a new Instant Answer after all? It can re-use the code you've written, and the regex would be a lot simpler. Also:

  • The main file should be renamed to Random.pm and put in a new lib/DDG/Spice/Bible directory
  • The namespace at the top of that file should be DDG::Spice::Bible::Random

I think the test file and JS file are OK as they are.

@pnpninja pnpninja mentioned this pull request Jul 31, 2016
@gerlachry gerlachry mentioned this pull request Aug 3, 2016
6 tasks
@moollaza
Copy link
Member

This PR is being closed due to inactivity. If you're still interested in finishing, please let me know and I'll gladly re-open it for you!

@moollaza moollaza closed this Jul 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants