zeitgeist team mailing list archive
-
zeitgeist team
-
Mailing list archive
-
Message #02064
Re: [Merge] lp:~zeitgeist/zeitgeist/fix-655164 into lp:zeitgeist
On Mon, Oct 11, 2010 at 4:20 PM, Markus Korn <thekorn@xxxxxx> 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.
Follow ups
References
