Re: [Tracker] Anonymous File Nodes branch review

On 03/02/10 12:51, Philip Van Hoof wrote:
On Wed, 2010-02-03 at 12:08 +0000, Martyn Russell wrote:
-       tracker_sparql_builder_object_iri (sparql, uri);
+       tracker_sparql_builder_object (sparql, "_:foo");

Looks right to me

Just to repeat what I said on IRC:

Technically there is nothing wrong with "_:foo", however, the "foo" part is used in the generation of the id and it isn't professional or setting a good example to other apps that copy our code - we should use something a little more self explanatory IMO.

Shouldn't that be \"nie:DataObject\" ?do . \"nie:url\" ...

Don't be confused, this isn't a SPARQL query but a native SQLite query.

There's a dot before "nie:url" because "nie:DataObject" is the table
name and "nie:url" is the column name. Without that dot it would not be
a correct native SQLite query.

Ah yes, of course :) thanks. The rules all change when it becomes SQL :D

6. I think we should add proper error handling here:

+          /* For fallback to the old behaviour, we could do this here:
+           * resource_id = tracker_data_query_resource_id (url); */
+          return;

No there's no error in that case. It just means that no removing needs
to take place (we can silently ignore it).

OK, I got the impression from the code there was, but if this is normal then good :)

7. Hmm, not sure why this change was added, usually if result is set and
  >  0 in length then there was no error, checking that here seems
pointless to me as we would still check the array and its length even if
there was no error so? This is in
src/tracker-writeback/tracker-writeback-consumer.c in sparql_query_cb().

-       if (result&&  result->len>  0) {
+       if (!error&&  result&&  result->len>  0) {

I had the situation where error was set and result wasn't NULL. I think
result is left untouched in case error is set, and if then the stack
variable isn't cleared to NULL, error isn't NULL. So just checking for
result isn't sufficient.

I see. Generally in this case I handle error in a separate situation then, i.e. if (error) { ...; return; } and do the result stuff after, but if this fixed a problem great.

8. Again, I would make queries like this easier to read for maintainability:

What is hard about this? Note that only the "'%s'" and the
"nie:url ?url" got added tot he original query.

Nothing hard at all, it is simply quicker and easier to read and that's always an improvement for maintainability.

+ query = g_strdup_printf ("SELECT ?url '%s' ?predicate ?object {"
+                          "<%s>  ?predicate ?object ; nie:url ?url ."
+                          "  ?predicate tracker:writeback true "
+                          "}", data->subject, data->subject);

To me this:

query = g_strdup_printf ("SELECT ?url '%s' ?predicate ?object {"
                          "  <%s> ?predicate ?object ;"
                          "  nie:url ?url ."
                          "  ?predicate tracker:writeback true "

is clearer than this:

query = g_strdup_printf ("SELECT ?url '%s' ?predicate ?object {"
                          "<%s>  ?predicate ?object ; nie:url ?url ."
                          "  ?predicate tracker:writeback true "
                          "}", data->subject, data->subject);

Also, I presume this is valid SPARQL to put '%s' after ?url, looks to me

Yes, that's correct SPARQL

OK, I assumed it was.

like it will just be included as one of the columns in the results -
isn't that a little inefficient (depending on how many results are

There's no inefficiency caused by this. And doing it differently will
break the writeback modules and make things *a lot* harder to port and
support (all writeback modules need a rewrite).

I see, but really we only have a few now so sooner is better than later to fix something like this. It is only an efficiency. Really if we expect a lot of results to be returned here we might want to consider changing this.


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