[Tracker] miner-flickr-review branch review

[Martyn Russell <martyn lanedo com>]

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"


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


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 :)

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


5. Do we writeback nao:description? I guess this is something Flickr 
puts next to each picture?


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.


7. Please space out calculations:

+ private static const uint PULL_INTERVAL = 5*60; /* seconds */

should be "5 * 60"


8. Shouldn't progress be 0.0 initially and status "Initializing" ?


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


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


11. What does this do:
â+       delete_query = ("delete { <%1$s> dc:title ?title }"
+                    +  "where  { <%1$s> dc:title ?title }")
+                    .printf (photoset_urn);


12. Is dc:title right? shouldn't that be nie:title?


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?


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


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.


16. How does yield work?


17. Shouldn't we set the status in error conditions like when we don't 
get the right response from Rest?


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) {


19. I would prefer it if writeback was done in a different vala file so 
we keep the mining and writeback clearly apart.


20. I see we use nao:identifier? why?


21. This update_tripple_* API feels like it is duplicating code, I 
wonder if we could improve that in the lower APIs


22. Can we instead use #ifndef G_OS_WIN32 or put a #error in here? â

+#if G_OS_WIN32
+#else


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.


24. Generally we need more status updates I feel.

--

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.

--
Regards,
Martyn