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

New Week goodie added along with a t file #210

Merged
merged 3 commits into from
Aug 19, 2013

Conversation

gsquire
Copy link
Collaborator

@gsquire gsquire commented Aug 11, 2013

@majuscule
Copy link
Collaborator

Hi @gsquire!

Thanks for writing a plugin! Unfortunately, your test cases are not passing for me:

t/Week.t .. 1/? 
#   Failed test 'Expected result but dont get one on week 43 1984'
#   at /home/dylan/perl5/lib/perl5/DDG/Test/Goodie.pm line 65.

#   Failed test 'Expected result but dont get one on week 8 1956'
#   at /home/dylan/perl5/lib/perl5/DDG/Test/Goodie.pm line 65.

#   Failed test 'Expected result but dont get one on week 21 1987'
#   at /home/dylan/perl5/lib/perl5/DDG/Test/Goodie.pm line 65.
# Looks like you failed 3 tests of 7.
t/Week.t .. Dubious, test returned 3 (wstat 768, 0x300)
Failed 3/7 subtests 

Test Summary Report
-------------------
t/Week.t (Wstat: 768 Tests: 7 Failed: 3)
  Failed tests:  5-7
  Non-zero exit status: 3
Files=1, Tests=7,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.21 cusr  0.02 csys =  0.25 CPU)
Result: FAIL

Are they passing for you? Please let me know if so and I'll take a closer look, or let me know what in particular you're having trouble with.

Thanks!

@gsquire
Copy link
Collaborator Author

gsquire commented Aug 12, 2013

Hi @nospampleasemam, that's weird they didn't work for you. Here is my output I got on my run:

ok 1 - Regexp check against text for week current
ok 2 - Testing query week current
ok 3 - Regexp check against text for week 6
ok 4 - Testing query week 6
ok 5 - Testing query week 43 1984
ok 6 - Testing query week 8 1956
ok 7 - Testing query week 21 1987
1..7

I ran the test with the command perl -Ilib t/Week.t. That is a dash eye. I wonder why it didn't work for you. Should I push my test file again?

@majuscule
Copy link
Collaborator

It looks like there's a typo on line 47 in the regex repetition notation. 5ac6ceb. After fixing this, all tests passed.

So it looks good! But I have two suggestions. The first is that I'd like the results to be full sentences. I think something in the form "The 43rd week of 1984 began on March 8th." would sound best. To get the ordinal form of a number (i.e. "43rd"), you can use Lingua::EN::Numbers::Ordinate, which is already an included dependency (you can see an example of it in use in POTUS.pm). I'd also like to support more organic queries, for example: "what was the 5th week of 1943".

Tell me what you think!

Thanks

@gsquire
Copy link
Collaborator Author

gsquire commented Aug 14, 2013

@nospampleasemam Sorry about the typo, I changed the code on my machine but not here correctly. Anyways, I think those are good ideas. I suppose my terse commands are a less likely to be remembered by someone to type it in.

I will look into using that other module as well as changing up the query matching to use a regex instead. As you can tell I am new to this!

Cheers

@gsquire
Copy link
Collaborator Author

gsquire commented Aug 15, 2013

@nospampleasemam I updated the code with some more natural queries. The only thing I can think of now that might need to be changed is allowing white space in the regular expressions. Thanks!

@majuscule
Copy link
Collaborator

Hi @gsquire,

Thank you for making those changes! I hope you don't mind - I have made some substantial changes to your code. The first change I made was to the trigger. Instead of using two phrases to trigger on, the plugin now triggers on the word "week". We then use a regex immediately inside the handler function to describe all of the different formations of the query phrase. Doing it this way, it allows us to keep all of the trigger parsing in one place, rather than the three if statements you used. Next, I moved the months hash you had outside of the handle function. This way, it is only instantiated once, rather than each time the handler is run on a query. I also changed it to an array, since you were only using numeric, sequential keys (which are inherent to an array anyway). After that, I changed the DateTime object to take the users timezone, rather than the timezone of the server that this code is running on. This was done with the $loc variable (which is describe here: https://github.com/duckduckgo/duckduckgo/blob/master/documentation/location_api.md). Finally, I used the ordsuf function of the Lingua::En::Numbers::Ordinate package, rather than just ordinate. I did this because it gives us just the suffix alone, which allowed me to place the suffix inside <sup> markup for HTML display. After doing all of this, I added some new test cases, that test both the new query phrases supported, and also the HTML returned.

I know that these are a lot of changes, but please take a look over them and tell me if everything makes sense. If anything doesn't - ask me! I'll be happy to explain. In the meantime, I have deployed the plugin on dylan.duckduckgo.com (https://dylan.duckduckgo.com/?q=what+week+is+this) to look at, and I am merging the code into our master repository.

Thanks again for writing and submitting a goodie! I really hope we see another pull request from you in the future! :-)

Dylan

@majuscule majuscule merged commit 91c64d7 into duckduckgo:master Aug 19, 2013
@gsquire
Copy link
Collaborator Author

gsquire commented Oct 1, 2013

@nospampleasemam I searched around for your email but to no avail. I was just curious when this instant answer would be live on DDG? I was looking forward to seeing it work :). Thanks.

@yackx
Copy link
Contributor

yackx commented Oct 30, 2014

So it's been there for one year but it's still not available live? 😩

@majuscule
Copy link
Collaborator

Hi @YouriAckx,

I'm very sorry about that! I'm not sure why it never got deployed, but I've pinged @moollaza and @jagtalon about this - hopefully we can get it live very quickly.

Thanks again and my sincere apologies,
Dylan

@moollaza
Copy link
Member

There is a new task to get this reviewed and deployed live #712

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.

4 participants