On Mon, Oct 11, 2010 at 4:20 PM, Markus Korn <thek...@gmx.de> wrote:

> I did not have time yet to check if the tests make sense, but let me give
> you a few comments first:
>
>
> > === modified file '_zeitgeist/engine/main.py'
> > --- _zeitgeist/engine/main.py   2010-09-29 08:39:32 +0000
> > +++ _zeitgeist/engine/main.py   2010-10-10 14:57:44 +0000
> > @@ -370,7 +370,15 @@
> >                        " GROUP BY subj_origin ORDER BY timestamp ASC",
> >                        " GROUP BY subj_origin ORDER BY COUNT(subj_origin)
> DESC, timestamp DESC",
> >                        " GROUP BY subj_origin ORDER BY COUNT(subj_origin)
> ASC, timestamp ASC",
> > -                       " GROUP BY actor ORDER BY timestamp ASC")[order]
> > +                       " GROUP BY actor ORDER BY timestamp ASC",
> > +                       " GROUP BY subj_interpretation ORDER BY timestamp
> DESC",
> > +                       " GROUP BY subj_interpretation ORDER BY timestamp
> ASC",
> > +                       " GROUP BY subj_interpretation ORDER BY
> count(subj_interpretation) DESC",
> > +                       " GROUP BY subj_interpretation ORDER BY
> count(subj_interpretation) ASC",
> > +                       " GROUP BY subj_mimetype ORDER BY timestamp
> DESC",
> > +                       " GROUP BY subj_mimetype ORDER BY timestamp ASC",
> > +                       " GROUP BY subj_mimetype ORDER BY
> count(subj_mimetype) DESC",
> > +                       " GROUP BY subj_mimetype ORDER BY
> count(subj_mimetype) ASC")[order]
> >
> >                if max_events > 0:
> >                        sql += " LIMIT %d" % max_events
>
> * we should use the same sql conventions for all statements, so please
> change count(...) to COUNT(...)
>
fixed

> * I personally don't like this big tuple containing the GROUP BY/ORDER BY
> statements, I suggest moving this tuple to a module wiede variable
> RESULT_TYPE_STATEMENTS. Also, the more resulttypes we add, the uglier
> (harder to read) this tuple gets. Maybe it's time to change this tuple to a
> dict now, like
>  {ResultType.MostRecentEvents: "ORDER BY ...",
> ResultType.LeastRecentEvents: ...}
>
I agree with you there but I think this is a bug request of its own so I
wont fix it in this branch

>
> >
> > === modified file 'test/data/twenty_events.js'
> > --- test/data/twenty_events.js  2010-07-28 20:10:07 +0000
> > +++ test/data/twenty_events.js  2010-10-10 14:57:44 +0000
> > @@ -23,10 +23,10 @@
> >                "subjects" : [
> >                        {
> >                                "uri" : "file:///tmp/foo.txt",
> > -                               "interpretation" : "stfu:Document",
> > +                               "interpretation" : "stfu:Video",
> >                                "manifestation" : "stfu:File",
> >                                "origin" : "file:///tmp",
> > -                               "mimetype" : "text/plain",
> > +                               "mimetype" : "text/booboo",
> >                                "text" : "this item has not text... rly!",
> >                                "storage" :
> "368c991f-8b59-4018-8130-3ce0ec944157"
> >                        }
> > @@ -42,7 +42,7 @@
> >                                "interpretation" : "stfu:Document",
> >                                "manifestation" : "stfu:File",
> >                                "origin" : "file:///tmp",
> > -                               "mimetype" : "text/plain",
> > +                               "mimetype" : "text/looloo",
> >                                "text" : "this item has not text... rly!",
> >                                "storage" :
> "368c991f-8b59-4018-8130-3ce0ec944157"
> >                        }
> > @@ -55,10 +55,10 @@
> >                "subjects" : [
> >                        {
> >                                "uri" : "file:///tmp/foo.txt",
> > -                               "interpretation" : "stfu:Document",
> > +                               "interpretation" : "stfu:Video",
> >                                "manifestation" : "stfu:File",
> >                                "origin" : "file:///tmp",
> > -                               "mimetype" : "text/plain",
> > +                               "mimetype" : "text/looloo",
> >                                "text" : "this item has not text... rly!",
> >                                "storage" :
> "368c991f-8b59-4018-8130-3ce0ec944157"
> >                        }
> > @@ -103,7 +103,7 @@
> >                "subjects" : [
> >                        {
> >                                "uri" : "file:///tmp/foo.txt",
> > -                               "interpretation" : "stfu:Document",
> > +                               "interpretation" : "stfu:Video",
> >                                "manifestation" : "stfu:File",
> >                                "origin" : "file:///tmp",
> >                                "mimetype" : "text/plain",
> > @@ -122,7 +122,7 @@
> >                                "interpretation" : "stfu:Document",
> >                                "manifestation" : "stfu:File",
> >                                "origin" : "file:///tmp",
> > -                               "mimetype" : "text/plain",
> > +                               "mimetype" : "text/booboo",
> >                                "text" : "this item has not text... rly!",
> >                                "storage" :
> "368c991f-8b59-4018-8130-3ce0ec944157"
> >                        }
> > @@ -135,7 +135,7 @@
> >                "subjects" : [
> >                        {
> >                                "uri" : "file:///tmp/foo.txt",
> > -                               "interpretation" : "stfu:Document",
> > +                               "interpretation" : "stfu:Music",
> >                                "manifestation" : "stfu:File",
> >                                "origin" : "file:///tmp",
> >                                "mimetype" : "text/plain",
> > @@ -151,7 +151,7 @@
> >                "subjects" : [
> >                        {
> >                                "uri" : "file:///tmp/foo.txt",
> > -                               "interpretation" : "stfu:Document",
> > +                               "interpretation" : "stfu:Music",
> >                                "manifestation" : "stfu:File",
> >                                "origin" : "file:///tmp",
> >                                "mimetype" : "text/plain",
> > @@ -170,7 +170,7 @@
> >                                "interpretation" : "stfu:Document",
> >                                "manifestation" : "stfu:File",
> >                                "origin" : "file:///tmp",
> > -                               "mimetype" : "text/plain",
> > +                               "mimetype" : "text/looloo",
> >                                "text" : "this item has not text... rly!",
> >                                "storage" :
> "368c991f-8b59-4018-8130-3ce0ec944157"
> >                        }
> > @@ -231,10 +231,10 @@
> >                "subjects" : [
> >                        {
> >                                "uri" : "file:///tmp/foo.txt",
> > -                               "interpretation" : "stfu:Document",
> > +                               "interpretation" : "stfu:Music",
> >                                "manifestation" : "stfu:File",
> >                                "origin" : "file:///tmp",
> > -                               "mimetype" : "text/plain",
> > +                               "mimetype" : "text/wumbo",
> >                                "text" : "this item has not text... rly!",
> >                                "storage" :
> "368c991f-8b59-4018-8130-3ce0ec944157"
> >                        }
> > @@ -263,7 +263,7 @@
> >                "subjects" : [
> >                        {
> >                                "uri" : "file:///tmp/foo.txt",
> > -                               "interpretation" : "stfu:Document",
> > +                               "interpretation" : "stfu:Music",
> >                                "manifestation" : "stfu:File",
> >                                "origin" : "file:///tmp",
> >                                "mimetype" : "text/plain",
> > @@ -295,7 +295,7 @@
> >                "subjects" : [
> >                        {
> >                                "uri" : "file:///tmp/foo.txt",
> > -                               "interpretation" : "stfu:Document",
> > +                               "interpretation" : "stfu:Something",
> >                                "manifestation" : "stfu:File",
> >                                "origin" : "file:///home",
> >                                "mimetype" : "text/plain",
> >
> > === modified file 'test/engine-test.py'
> > --- test/engine-test.py 2010-09-25 13:19:51 +0000
> > +++ test/engine-test.py 2010-10-10 14:57:44 +0000
> > @@ -688,6 +688,62 @@
> >                events = self.engine.find_events(
> >                        TimeRange.always(), [], StorageState.Any, 0,
> ResultType.LeastRecentOrigin)
> >                self.assertEquals([e[0][1] for e in events], ["116",
> "118", "119"])
> > +
> > +       def testResultTypesMostRecentMimetType(self):
> > +               import_events("test/data/twenty_events.js", self.engine)
> > +
> > +               events = self.engine.find_events(
> > +                       TimeRange.always(), [], StorageState.Any, 0,
> ResultType.MostRecentMimeType)
> > +               self.assertEquals([e[0][1] for e in events], ['119',
> '114', '110', '107'])
> > +
> > +       def testResultTypesLeastRecentMimetType(self):
> > +               import_events("test/data/twenty_events.js", self.engine)
> > +
> > +               events = self.engine.find_events(
> > +                       TimeRange.always(), [], StorageState.Any, 0,
> ResultType.LeastRecentMimeType)
> > +               self.assertEquals([e[0][1] for e in events], ['107',
> '110', '114', '119'])
> > +
> > +       def testResultTypesMostPopularMimetType(self):
> > +               import_events("test/data/twenty_events.js", self.engine)
> > +
> > +               events = self.engine.find_events(
> > +                       TimeRange.always(), [], StorageState.Any, 0,
> ResultType.MostPopularMimeType)
> > +               self.assertEquals([e[0][1] for e in events], ['119',
> '110', '107', '114'])
> > +
> > +       def testResultTypesLeastPopularMimetType(self):
> > +               import_events("test/data/twenty_events.js", self.engine)
> > +
> > +               events = self.engine.find_events(
> > +                       TimeRange.always(), [], StorageState.Any, 0,
> ResultType.LeastPopularMimeType)
> > +               self.assertEquals([e[0][1] for e in events], ['114',
> '107', '110', '119'])
> > +
> > +       def testResultTypesMostRecentSubjectInterpretation(self):
> > +               import_events("test/data/twenty_events.js", self.engine)
> > +
> > +               events = self.engine.find_events(
> > +                       TimeRange.always(), [], StorageState.Any, 0,
> ResultType.MostRecentSubjectInterpretation)
> > +               self.assertEquals([e[0][1] for e in events], ['119',
> '118', '116', '106'])
> > +
> > +       def testResultTypesLeastRecentSubjectInterpretation(self):
> > +               import_events("test/data/twenty_events.js", self.engine)
> > +
> > +               events = self.engine.find_events(
> > +                       TimeRange.always(), [], StorageState.Any, 0,
> ResultType.LeastRecentSubjectInterpretation)
> > +               self.assertEquals([e[0][1] for e in events], ['106',
> '116', '118', '119'])
> > +
> > +       def testResultTypesMostPopularSubjectInterpretation(self):
> > +               import_events("test/data/twenty_events.js", self.engine)
> > +
> > +               events = self.engine.find_events(
> > +                       TimeRange.always(), [], StorageState.Any, 0,
> ResultType.MostPopularSubjectInterpretation)
> > +               self.assertEquals([e[0][1] for e in events], ['119',
> '116', '106', '118'])
> > +
> > +       def testResultTypesLeastPopularSubjectInterpretation(self):
> > +               import_events("test/data/twenty_events.js", self.engine)
> > +
> > +               events = self.engine.find_events(
> > +                       TimeRange.always(), [], StorageState.Any, 0,
> ResultType.LeastPopularSubjectInterpretation)
> > +               self.assertEquals([e[0][1] for e in events], ['118',
> '106', '116', '119'])
> >
> >        def testRelatedForEventsSortRelevancy(self):
> >                import_events("test/data/apriori_events.js", self.engine)
>
> * I know it is done similar in other tests, but we should make tests good
> examples of how to use zeitgeist, so please change e[0][1] to e.timestamp
>
fixed

>
>
> >
> > === modified file 'zeitgeist/datamodel.py'
> > --- zeitgeist/datamodel.py      2010-09-25 13:19:51 +0000
> > +++ zeitgeist/datamodel.py      2010-10-10 14:57:44 +0000
> > @@ -1024,7 +1024,7 @@
> >        MostPopularSubjects = enum_factory(("One event for each subject
> only, "
> >                "ordered by the popularity of the subject"))
> >        LeastPopularSubjects = enum_factory(("One event for each subject
> only, "
> > -               "ordered ascendingly by popularity"))
> > +               "ordered ascendingly by popularity of the mimetype"))
>
> Huh? how is mimetype related to LeastPopularSubjects?

fixed

>

>        MostPopularActor = enum_factory(("The last event of each different
> actor,"
> >                "ordered by the popularity of the actor"))
> >        LeastPopularActor = enum_factory(("The last event of each
> different actor,"
> > @@ -1037,7 +1037,23 @@
> >                "ordered by the popularity of the origins"))
> >        LeastPopularOrigin = enum_factory(("The last event of each
> different origin,"
> >                "ordered ascendingly by the popularity of the origin"))
> > -       OldestActor = enum_factory(("The first event of each different
> actor"))
> > +       OldestActor = enum_factory(("The first event of each different
> actor"))
> > +       MostRecentSubjectInterpretation = enum_factory(("One event for
> each subject interpretation only, "
> > +               "ordered with the most recent events first"))
> > +       LeastRecentSubjectInterpretation = enum_factory(("One event for
> each subject interpretation only, "
> > +               "ordered with the least recent events first"))
> > +       MostPopularSubjectInterpretation = enum_factory(("One event for
> each subject interpretation only, "
> > +               "ordered by the popularity of the subject
> interpretation"))
> > +       LeastPopularSubjectInterpretation = enum_factory(("One event for
> each subject interpretation only, "
> > +               "ordered ascendingly by popularity of the subject
> interpretation"))
> > +       MostRecentMimeType = enum_factory(("One event for each mimetype
> only, "
> > +               "ordered with the most recent events first"))
> > +       LeastRecentMimeType = enum_factory(("One event for each mimetype
> only, "
> > +               "ordered with the least recent events first"))
> > +       MostPopularMimeType = enum_factory(("One event for each mimetype
> only, "
> > +               "ordered by the popularity of the mimetype"))
> > +       LeastPopularMimeType = enum_factory(("One event for each mimetype
> only, "
> > +               "ordered ascendingly by popularity of the mimetype"))
> >
>
> I'm always shockend when reading this part of datamodel.py, it always takes
> minutes for me to find the right ResultType, but this is not related to your
> changes here as we agreed on the structure of ResultType a few days ago.

Again this requires a separate bug report to be dealt with.


> --
> https://code.launchpad.net/~zeitgeist/zeitgeist/fix-655164/+merge/38077
> You proposed lp:~zeitgeist/zeitgeist/fix-655164 for merging.
>

OK please review again

-- 
This is me doing some advertisement for my blog http://seilo.geekyogre.com

https://code.launchpad.net/~zeitgeist/zeitgeist/fix-655164/+merge/38077
Your team Zeitgeist Framework Team is requested to review the proposed merge of 
lp:~zeitgeist/zeitgeist/fix-655164 into lp:zeitgeist.

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

Reply via email to