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(...)
* 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: 
...}


>
> === 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


>
> === 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?

>        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.

-- 
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