Created attachment 292850 [details] [review]
lua-factory: Add hack for running tests on lua sources

Same snippet of code as in the local-metadata plugin.

Created attachment 292851 [details] [review]
WIP: Use lua to parse video titles

Instead of regexes in local-metadata

Comment on attachment 292850 [details] [review]
lua-factory: Add hack for running tests on lua sources

bug 741784 had a similar patch.

Created attachment 298194 [details] [review]
lua source: video-title-parsing

Using the lua patterns to the job :-)
I think this is easier to read and fix when comparing to the regexp parser. 

One difference here is that, this source is parsing the title but maybe it should do for URIs too?

I changed the local-metadata to test both local-metadata and the lua source... but I'll include the tests properly after fixing #741784

Review of attachment 298194 [details] [review]:

No need to mention me. I would prefer a better explanation in the commit message though.

Does it pass the test suite now?

::: tests/local-metadata/test_local_metadata.c
@@ +116,3 @@
 
+  registry = grl_registry_get_default ();
+  source = grl_registry_lookup_source (registry, "grl-video-title-parsing");

Isn't the source name the only difference between the 2 test functions? If so, factor it out.

(In reply to Bastien Nocera from comment #5)
> Review of attachment 298194 [details] [review] [review]:
> 
> No need to mention me. I would prefer a better explanation in the commit
> message though.

Sure, something like below is fine?

"video-title-parsing: lua patterns instead of regex

This source is an alternative to local-metadata and it uses lua patterns in order to get metadata from the title of the file.

The test suite for video-title-parsing is the same for the local-metadata"

> 
> Does it pass the test suite now?

It does! (for titles, not URIs)

> 
> ::: tests/local-metadata/test_local_metadata.c
> @@ +116,3 @@
>  
> +  registry = grl_registry_get_default ();
> +  source = grl_registry_lookup_source (registry, "grl-video-title-parsing");
> 
> Isn't the source name the only difference between the 2 test functions? If
> so, factor it out.

I'm also checking if the array of test case has title before calling the resolve function. (sometimes it only has URIs)

I separated in two functions to have two different tests, not sure how to do it properly with one function and a global GrlSource.

Is it okay to leave this [lua source] test inside local-metadata tests?

Also, should this source handle URIs too?

(In reply to Victor Toso from comment #6)
> (In reply to Bastien Nocera from comment #5)
> > Review of attachment 298194 [details] [review] [review] [review]:
> > 
> > No need to mention me. I would prefer a better explanation in the commit
> > message though.
> 
> Sure, something like below is fine?
> 
> "video-title-parsing: lua patterns instead of regex

"lua-factory: Add title parsing Lua source"

> This source is an alternative to local-metadata and it uses lua patterns in
> order to get metadata from the title of the file.
> 
> The test suite for video-title-parsing is the same for the local-metadata"
> 
> > 
> > Does it pass the test suite now?
> 
> It does! (for titles, not URIs)
> 
> > 
> > ::: tests/local-metadata/test_local_metadata.c
> > @@ +116,3 @@
> >  
> > +  registry = grl_registry_get_default ();
> > +  source = grl_registry_lookup_source (registry, "grl-video-title-parsing");
> > 
> > Isn't the source name the only difference between the 2 test functions? If
> > so, factor it out.
> 
> I'm also checking if the array of test case has title before calling the
> resolve function. (sometimes it only has URIs)
> 
> I separated in two functions to have two different tests, not sure how to do
> it properly with one function and a global GrlSource.
> 
> Is it okay to leave this [lua source] test inside local-metadata tests?
> 
> Also, should this source handle URIs too?

void
my_real_test(const char *source_name,
             gboolean check_for_uris)
{
   ...
}

And call this from the 2 different tests?

Created attachment 298293 [details] [review]
v2

> > "video-title-parsing: lua patterns instead of regex
> 
> "lua-factory: Add title parsing Lua source"

Fixed.

> 
> > This source is an alternative to local-metadata and it uses lua patterns in
> > order to get metadata from the title of the file.
> > 
> > The test suite for video-title-parsing is the same for the local-metadata"
> > 
> > > 
> > > Does it pass the test suite now?
> > 
> > It does! (for titles, not URIs)
> > 
> > > 
> > > ::: tests/local-metadata/test_local_metadata.c
> > > @@ +116,3 @@
> > >  
> > > +  registry = grl_registry_get_default ();
> > > +  source = grl_registry_lookup_source (registry, "grl-video-title-parsing");
> > > 
> > > Isn't the source name the only difference between the 2 test functions? If
> > > so, factor it out.
> > 
> > I'm also checking if the array of test case has title before calling the
> > resolve function. (sometimes it only has URIs)
> > 
> > I separated in two functions to have two different tests, not sure how to do
> > it properly with one function and a global GrlSource.
> > 
> > Is it okay to leave this [lua source] test inside local-metadata tests?
> > 
> > Also, should this source handle URIs too?
> 
> void
> my_real_test(const char *source_name,
>              gboolean check_for_uris)
> {
>    ...
> }
> 
> And call this from the 2 different tests?

Thanks

Review of attachment 298293 [details] [review]:

Looks good!

We should add support for URIs as well. We will probably need to add a helper in lua-factory to handle that though, to be sure that the behaviour is the same as the Filesystem or Tracker plugins.

pushed as ba849446abcc512a7cff07b3e975d409139bc898
(not very lucky with git-bz lately)

(In reply to Bastien Nocera from comment #10)
> Review of attachment 298293 [details] [review] [review]:
> 
> Looks good!
> 
> We should add support for URIs as well. We will probably need to add a
> helper in lua-factory to handle that though, to be sure that the behaviour
> is the same as the Filesystem or Tracker plugins.

Indeed, I'll try to do it this week.