GNOME Bugzilla – Bug 732261
local-metadata: new regexp match
Last modified: 2014-06-30 13:04:40 UTC
Case: Felicity 4.08 (Last Thanksgiving).avi Match: season 4, episode 8 and title is everything inside ();.
Created attachment 279280 [details] [review] local-metadata: new match
I forgot to mention that the patch includes the test case.
Review of attachment 279280 [details] [review]: ::: src/local-metadata/grl-local-metadata.c @@ +311,3 @@ /* Replace "." with <space> */ g_strdelimit (*title, ".", ' '); + *title = g_strstrip (*title); That doesn't look related, do this in a separate patch. @@ +350,3 @@ *title = g_match_info_fetch_named (info, "name"); + g_strdelimit (*title, ".()", ' '); + *title = g_strstrip (*title); Ditto. @@ +356,3 @@ *showname = g_match_info_fetch_named (info, "showname"); g_strdelimit (*showname, ".", ' '); + *showname = g_strstrip (*showname); Ditto. ::: tests/local-metadata/test_local_metadata.c @@ +97,3 @@ { NULL, "file:///home/test/My%20super%20series.S01E01.mp4", "My super series", 1, 1 }, { "Adventure Time - 2x01 - It Came from the Nightosphere.mp4", NULL, "Adventure Time", 2, 1 }, + { NULL, "file:///home/toso/Downloads/Felicity/Felicity%202.05%20(Crash).avi", "Felicity", 2, 5 }, Can you please add a test case for the new matches, separately?
Created attachment 279316 [details] [review] local-metadata: new match
Created attachment 279317 [details] [review] local-metadata: fix spaces on metadata
Created attachment 279318 [details] [review] local-metadata: add test of new matching case
Review of attachment 279316 [details] [review]: Looks fine.
Review of attachment 279317 [details] [review]: Looks good
Review of attachment 279318 [details] [review]: ::: tests/local-metadata/test_local_metadata.c @@ +97,3 @@ { NULL, "file:///home/test/My%20super%20series.S01E01.mp4", "My super series", 1, 1 }, { "Adventure Time - 2x01 - It Came from the Nightosphere.mp4", NULL, "Adventure Time", 2, 1 }, + { NULL, "file:///home/toso/Downloads/Felicity/Felicity%202.05%20(Crash).avi", "Felicity", 2, 5 }, Can you please add separate test cases for the "." separated season/episode numbers and for the parenthesis-wrapped episode title?
Thanks for the review! I've included in the tests a check on the new-title of the media, after resolving. With this new test, I could verify a problem due case sensitive comparasion on strstr when the source is removing black-listed words such as DVDRip. I've included a fix for that too using strcasestr but that requires the #define _GNU_SOURCE and I'm not sure if that is fine.
Created attachment 279354 [details] [review] local-metadata: new match
Created attachment 279355 [details] [review] local-metadata: fix spaces on metadata
Created attachment 279356 [details] [review] local-metadata: fix to case insensitive on black list of words
Created attachment 279357 [details] [review] local-metadata: add test of new matching case New the test case using a dot to separate season to episode and the episode name between parentheses.
Review of attachment 279354 [details] [review]: ++
Review of attachment 279355 [details] [review]: ++
Review of attachment 279356 [details] [review]: blacklist is one word. Please also add a test case for this.
Review of attachment 279357 [details] [review]: The code doesn't seem to match the commit message. The code seems to be about testing for the updated title.
(In reply to comment #17) > Review of attachment 279356 [details] [review]: > > blacklist is one word. Please also add a test case for this. Thanks. I've mistaken in bugzilla. As test case, we already have the: "metalocalypse.s02e01.dvdrip.xvid-ffndvd.avi". It found 'dvdrip' as new title before the patch (should be NULL).
Review of attachment 279354 [details] [review]: up
Review of attachment 279355 [details] [review]: up
Created attachment 279569 [details] [review] local-metadata: fix to case insensitive on blacklist of words same as before, changing only bugzilla description
Created attachment 279570 [details] [review] local-metadata: check for updated title on tests Split last patch in two. This patch include a check in the new-title after resolving data with local-metadata source
Created attachment 279571 [details] [review] local-metadata: add test of new matching case I've included a comment in the code to specify the test.
Review of attachment 279569 [details] [review]: Looks good.
Review of attachment 279570 [details] [review]: Looks good.
Review of attachment 279571 [details] [review]: Fine.
By the way, use "git bz push" to push your patches, it'll be faster than changing the status of patches one-by-one.
Attachment 279570 [details] pushed as a7483e2 - local-metadata: check for updated title on tests