search

bookieio / Bookie

Python based delicious.com replacement
GNU Affero General Public License v3.0
633 stars 138 forks source link

Fix of Issue #528 #530

Closed yashmehrotra closed 10 years ago

yashmehrotra commented 10 years ago

I tested the case matching and it works

craigmaloney commented 10 years ago

Hi @yashmehrotra,

Thank you for this patch. Would you add a test case that checks that the tags are indeed matched? Ideally it would test upper, mixed, and lower case. It should fail without your patch applied.

Thanks again!

yashmehrotra commented 10 years ago

@craigmaloney Ok, I am adding them currenty , but I may need some help. At what time are you available on the IRC channel?

craigmaloney commented 10 years ago

@yashmehrotra I'm at work most of the day (EST) but should be available for a bit off-and-on.

yashmehrotra commented 10 years ago

Ok thank you

yashmehrotra commented 10 years ago

@craigmaloney I did what you said, I added tests and without my patch the test fails (and succeeds with the patch) . Kindly review my code. Thanks for the help.

mitechie commented 10 years ago

Thanks for the update! This looks really good. I think the test can be simplified and the code required there cut in half.

I also think that the better place to lowercase is in the BmarkMgr vs the view itself. Let me know what you think after looking into that a bit.

yashmehrotra commented 10 years ago

@mitechie Ok , I will make the relevant changes and put the case changing in BmarkMgr.find()

yashmehrotra commented 10 years ago

@mitechie Updated everything as you said . Thanks for the guidance.

craigmaloney commented 10 years ago

:+1:

yashmehrotra commented 10 years ago

@craigmaloney Thanks , so is it good for a merge?

yashmehrotra commented 10 years ago

@mitechie Converted it all into 1 commit .