[Tracker] miner-flickr-review branch review

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

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.


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