Code review comment for lp:~mhr3/libzeitgeist/data-source-registry

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

Outstanding work Michal! A few comments, but nothing critical:

 1) There is a comment "... coming from this Index instance ..." which should probably be s/Index/data source registry/

 2) You need a line break after the symbol name in the docstring for zeitgeist_data_source_registry_new()

 3) Can you add a bit of documentation to the ZeitgeistDataSource section?

 4) Can you document that you are stealing the ref to the GPtrArray in zeitgeist_data_source_set_templates()

 5) I prefer that getters returning booleans are named as foo_is_*(). This affects zeitgeist_data_source_get_{running,enabled}()

 6) Please add some unit tests for the conversion between the eggdbus stuff and the native types

 7) Integrate the two new types into the gtk-doc

If you fix this I'd be delighted to merge you branch :-)

And as a bonus you could perhaps add some more doc strings, but it's ok if you don't...

review: Needs Fixing

« Back to merge proposal