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

On Wed, 28 Apr 2010 11:22:49 +0000, 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:


1. Help string in 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, please remove the _deps part :)

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

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"

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() ?

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
the photoset title changed remotely, to refresh it)

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?

Added cast and spaced

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.
Yep, that could be factorized

16. How does yield work?
I invite you to look at

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


+  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.
Hmm yeah I understand that, but it's pretty hard to split the class
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
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
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
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.

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


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.

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]