Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/bug580364 into lp:zeitgeist

2010-08-31 Thread Markus Korn
Review: Disapprove
Sorry, big NACK from me.
It makes no sense to allow this kind of queries right now, as we have no ways 
to set the storage state implemented. This is why we decided to throw an 
exception.
-- 
https://code.launchpad.net/~zeitgeist/zeitgeist/bug580364/+merge/34141
Your team Zeitgeist Framework Team is subscribed to branch 
lp:~zeitgeist/zeitgeist/bug580364.

___
Mailing list: https://launchpad.net/~zeitgeist
Post to : zeitgeist@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zeitgeist
More help   : https://help.launchpad.net/ListHelp


Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/bug580364 into lp:zeitgeist

2010-08-31 Thread Mikkel Kamstrup Erlandsen
Review: Needs Fixing
Well, I disagree with Markus :-) I think it makes perfect sense to include this 
- otherwise we have a chicken-and-egg problem.

In theory anyone could set the storage field upon item insertion. It just so 
happens that the DS we ship is not feature complete. On top of that, 3rd 
parties could provide their own storage monitor extension which update the 
storage table correctly (at the very least a networkmanager/connman extension 
is easy).

Regarding the code:

 1) It looks like testFindStorageNotExistant() is there twice?

 2) Docstrings in the unit tests are not right

 3) Apart from these small things it looks good to me
-- 
https://code.launchpad.net/~zeitgeist/zeitgeist/bug580364/+merge/34141
Your team Zeitgeist Framework Team is subscribed to branch 
lp:~zeitgeist/zeitgeist/bug580364.

___
Mailing list: https://launchpad.net/~zeitgeist
Post to : zeitgeist@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zeitgeist
More help   : https://help.launchpad.net/ListHelp


Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/bug580364 into lp:zeitgeist

2010-08-31 Thread Markus Korn
Review: Needs Fixing
Ok, I cave in, fix the things mentioned by Mikkel, run the testsuite and if 
everything goes well merge it into lp:zeitgeist (but please don't *bzr pull*)
-- 
https://code.launchpad.net/~zeitgeist/zeitgeist/bug580364/+merge/34141
Your team Zeitgeist Framework Team is subscribed to branch 
lp:~zeitgeist/zeitgeist/bug580364.

___
Mailing list: https://launchpad.net/~zeitgeist
Post to : zeitgeist@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zeitgeist
More help   : https://help.launchpad.net/ListHelp