zeitgeist team mailing list archive
-
zeitgeist team
-
Mailing list archive
-
Message #02063
Re: [Merge] lp:~zeitgeist/zeitgeist/fix-655164 into lp:zeitgeist
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.
Follow ups
References
