Re: [Tracker] tracker-miner-rss 0.4.0 (and related stuffs)

On Fri, Mar 12, 2010 at 6:47 PM, Martyn Russell <martyn lanedo com> wrote:
On 04/03/10 14:43, Roberto Guido wrote:

1. Did you intend for miner-rss to be LGPL license? Doesn't make much sense
to me since it is primarily used for libraries. Perhaps you want GPL here.
My guess is this is a copy and paste error ;)

yes, a copy and paste error, fixed :)

2. tracker-main.c needs to include config.h like we do in all other source
files. Nothing is used from it yet, but it is something we do in all our
source files.


3. This is incorrect coding style:

+ main (int argc, char **argv) {

Please use:

+ main (...)
+ {


4. This should be tracker-common.h which includes all other headers to avoid
future incompatibility issues (tracker-rss-reader.c):

+ #include <libtracker-common/tracker-sparql-builder.h>

Also, headers should be in order of location, i.e. system --> higher level.
The "tracker-rss-reader.h" should be the last include generally.


5. Comments should have * on each line (noticed for TODO statements)


6. Generally we add the private struct after setting vtable properties.

+ g_type_class_add_private (object_class, sizeof (TrackerMinerRSSPrivate));

Ok, Done

7. These need lining up:

+ subjects_removed_cb (DBusGProxy *proxy,
+ Â Â Â Â Â Â Â Â Â Â Âgchar **subjects,
+ Â Â Â Â Â Â Â Â Â Â Âgpointer user_data)


8. This should be translated using _("Initializing")

+ g_object_set (object, "progress", 0.0, "status", "Initializing", NULL);


9. In tracker_miner_rss_init(), I would suggest you do the wrap != NULL
check all before you create memory for other things. To avoid memory
allocation where initialisation fails.


10. I see you're using:

+ double prog;

Please use gdouble there, gdouble is the type we expect for progress.


11. The "Fetching" status should be translated.


12. I do worry that there is no checking for the result or when the query
has finished here:

+ tracker_miner_execute_update (TRACKER_MINER (miner),
               Âtracker_sparql_builder_get_result (sparql),

It feels a bit unfinished there. Is more work needed here?


13. In our coding style, arrays don't have a space between the index and
name of the array:

+ if (g_strcmp0 (values [0], "1") == 0)


14. In item_verify_reply_cb(), uri should be freed.


15. I notice that tracker_sparql_builder_object_string() is used a lot, but
we should be using _object_unvalidated() for strings which need utf-8
checking. If we don't do this, you risk having your process exit() by d-bus.

Ok, Done

16. Coding style is incorrect here:

+ static const gchar*


17. I notice paused and resumed set the progress to 100%, that sounds
incorrect to me. Progress usually doesn't change in those states.



Generally, it looks really good. Just my nazi coding style issues to fix -
don't worry I am famous for it :)

The only thing I would add is, I think it would be useful to have some sort
of logging so progress and general debugging can be tracked more easily.


It might also make sense to have a config option to disable the miner-rss
(or perhaps do it some other way).

Not sure on how do this...

I would be happy to merge this branch if you can make all of pvanhoof's
changes and my changes.

We should have fixed all the things you and Philip told to us... hope
that now the miner is (almost) ready for hit the repo :)

Michele & Bob

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