On Wed, 28 Apr 2010 11:22:49 +0000, Martyn Russell <[email protected]>
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.
_______________________________________________
tracker-list mailing list
[email protected]
http://mail.gnome.org/mailman/listinfo/tracker-list
