Re: [Tracker] miner-flickr-review branch review

[Adrien Bustany <abustany gnome org>]

On Wed, 28 Apr 2010 11:22:49 +0000, Martyn Russell <martyn lanedo com>
wrote:
> Hi all,
>
> Adrien recently worked on the miner-flickr-review branch to add support 
> for extracting Flickr data and for writing the data back too. This is 
> the review of that branch:
>
>    http://git.gnome.org/browse/tracker/log/?h=miner-flickr-review
>
> --
>
> 1. Help string in configure.ac should be "--enable-miner-flickr"
Done

> 2. No need for space around "[ rest-0.6 >= $REST_REQUIRED ]"
Done

> 3. Don't call it have_miner_flickr_deps, we don't use that in the rest 
> of configure.ac, please remove the _deps part :)
Done

> 4. Summary shouldn't have "miner" in it, since only miners are listed
> there.
Done

> 5. Do we writeback nao:description? I guess this is something Flickr 
> puts next to each picture?
I don't get this one... I store pictures descriptions as nie:comment.
And AFAIK Flickr doesn't allow me to update a picture's description.

> 6. What are the API_KEY/SHARED_SECRET? They are defined but look like 
> some sort of random string. We should at least put a comment describing 
> where this comes from, how it was generated, etc.
Those are used by Flickr to identify the app. I added a comment for that.

> 7. Please space out calculations:
>
> + private static const uint PULL_INTERVAL = 5*60; /* seconds */
>
> should be "5 * 60"
Done

> 8. Shouldn't progress be 0.0 initially and status "Initializing" ?
Technically the miner won't do anything until you call authenticate,
so it's idle.

> 9. Shouldn't we set status in shutdown() ?
Done

> 10. Won't the init_datasource() error on every subsequent construct() 
> other than the first (since it tries to insert without checking)?
It doesn't error here

> 11. What does this do:
> â+       delete_query = ("delete { <%1$s> dc:title ?title }"
> +                    +  "where  { <%1$s> dc:title ?title }")
> +                    .printf (photoset_urn);
I remove the stored title, since it's going to be imported again (useful
if
the photoset title changed remotely, to refresh it)
> 12. Is dc:title right? shouldn't that be nie:title?
Changed

> 13. Please space things properly:
>
> + (1.0*indexed_photosets)/n_photosets;
>
> Also, why 1.0 * ? Do you mean to just cast this to a gdouble?
Added cast and spaced

> 14. Shouldn't rdfs:comment be nie:comment?
Changed

> 15. Exif stuff should probably be added to libtracker-extract? I see a 
> lot of duplicate code here where we check whitespace values, etc with 
> constants in SPARQL. I would prefer it if we simplified all this and had

> some API - I think libtracker-extract is the most likely place for such 
> a thing.
Yep, that could be factorized

> 16. How does yield work?
I invite you to look at
http://blogs.gnome.org/juergbi/2009/09/18/closures-and-asynchronous-methods-in-vala/
> 17. Shouldn't we set the status in error conditions like when we don't 
> get the right response from Rest?
Done

> 18. Please don't put conditional operators at the start of the line like

> this:â
>
> +  if (token_node == null || token_node.content == null
> +    || user_node == null || user_node.get_attr ("username") == null) {
Fixed

> 19. I would prefer it if writeback was done in a different vala file so 
> we keep the mining and writeback clearly apart.
Hmm yeah I understand that, but it's pretty hard to split the class
without
having to duplicate a lot of code :/ Or I can make the run_call public in
MinerFlickr and pass it to a MinerFlickrWriteback object maybe...

> 20. I see we use nao:identifier? why?
I need to keep a persistent mapping between urns and flickr identifiers. I
chose to use nao:identifier for that, but that could be stored in a
separate
file/DB too.

> 21. This update_tripple_* API feels like it is duplicating code, I 
> wonder if we could improve that in the lower APIs
Updating values is a real pain with SPARQL. So yeah, might make sense to
have
that in libtracker-client, at least until I do my summer of code ;)

> 22. Can we instead use #ifndef G_OS_WIN32 or put a #error in here? â
>
> +#if G_OS_WIN32
> +#else
I don't think so. But yeah that looks awkward as it is :)

> 23. We usually put a message in the signal_handler function so we know 
> what we got before we quit the main loop, we should add the same thing 
> to this miner.
Done

> 24. Generally we need more status updates I feel.
I have to find how I can emit them, since I'm doing a lot of things in a
synchronous
way.

> --
>
> Good work though Adrien, I have yet to test that it works, but looking 
> at your recent blog, it seems quite promising. I will try to find time 
> to test it soon.