=== jtv changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [noodles775,adiroiban(bug-564852), jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [10:41] No volunteers for a review today? === henninge_ is now known as henninge [12:15] jtv: Let's swap: https://code.edge.launchpad.net/~henninge/launchpad/bug-516736-templatename/+merge/23659 [12:16] henninge: OK! [12:20] henninge: s/\ henninge: also, it may help to introduce a variable in the last paragraph of the diff: [12:24] potemplate = self.context.potemplate [12:24] if potemplate is None: [12:24] # ... [12:24] else: [12:24] # Do stuff with potemplate, not self.context.potemplate [12:25] jtv: yeah, nice idea. Although one line lost, one line gained... [12:32] jtv: how did you "tighten the check a bit more" ? [12:34] henninge: besides just counting the number of forms, I'm also checking that they are the right forms. The old code would not detect the problem with nplurals=2 and "n ? 0 : 2" for instance. [12:34] oic [12:34] henninge: anyway, I'm approving yours. [12:34] jtv: is it this? [12:35] 144 - if pluralforms_count != len(forms): [12:35] 145 + found_forms = sorted(forms.keys()) [12:35] 146 + if found_forms != range(expected_forms): [12:35] henninge: right [12:35] cool [12:35] jtv: thanks [12:35] It may seem wasteful at first to do a range comparison, but this is only over the number of forms anyway—maximum of 6 [12:36] jtv: I just have two small things, I'll mention them in the review. r=me [12:37] henninge: dankeschöen [12:37] ahem [12:37] doppelt gemoppelt [12:37] ön [12:37] ;-) === henninge changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [noodles775,adiroiban(bug-564852)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [12:44] jtv: review sent [12:44] thanks === mrevell is now known as mrevell-lunch === mrevell-lunch is now known as mrevell [15:28] BjornT, ping [15:29] hi sinzui [15:30] I am not certain from your remarks about the not-packaged-1 branch whether I should try a different name for a new column, or proceed using Packaging [15:32] sinzui: it depends on your use case. if you actually want to record that something isn't packaged, and display this information in the ui, go for the Packaging table. [15:33] BjornT, regarding the enum, we hide the INCLUDE enum a few months ago because project owner do not understand it. It is used for some Debian and Ubuntu packages. While it is correct for modeling, it does not provide a lot of value for the project or ubuntu contributors [15:33] sinzui: if all you want to do is to control whether to ask the user whether it's packaged, go for the column with a different name [15:33] I will choose a different name since this is really about a portlet behaviour [15:34] sinzui: ok, cool === deryck is now known as deryck[lunch] === bac` is now known as BradCrittenden === BradCrittenden is now known as bac === deryck[lunch] is now known as deryck [16:46] danilos: Hi, when you have some time can you please take a look at the current interface proposed for bug 512735 and have a short preimplementation chat? [16:46] Bug #512735: At first diplay only preferred languages for a POFile from a POFileSubset [17:03] adiroiban, sure [17:03] adiroiban, should I look at the branch? [17:05] yes === matsubara is now known as matsubara-lunch [17:05] danilos: there is an IHasLangaugesWithTranslations interface [17:06] adiroiban, right, looking [17:06] and an implementation of language-statistics view [17:06] adiroiban, so, I am not sure this is quite enough; this is definitely a step in right direction though [17:06] danilos: what is not enough? the fields from the interface? [17:07] adiroiban, yeah, I am not sure that's completely enough [17:10] adiroiban, still looking though :) [17:10] adiroiban, the biggest concern is performance [17:11] adiroiban, also, I suspect you based the generic languages-translations-statistics.pt on one of distroseries or productseries templates [17:11] adiroiban, it would have been much easier to read that if you moved one of them to it [17:12] adiroiban, also, has_translations and languages_statistics should probably be @properties [17:19] danilos: true. my bad for not moving one of those files. And yes, those should be properties [17:22] danilos: the idea is that there will a lot of changes, so I should start by implementing an interface for distroseries and productseries and commit that one [17:23] and then see what other fields are required by sourcepakage and potemplate and add them later [17:24] adiroiban, sure, sounds good, that's why I am complaining about the templates, because that'd be the easiest way for me to see if this is indeed enough for the first go :) [17:25] adiroiban, btw, LanguageStatisticsView should definitely not be in browser/language.py :) [17:26] danilos: agree, I was not sure where to put it. Should I add it in a new file? [17:26] adiroiban, sure, please do... it's also not really "language_statistics", it's "per-language statistics" [17:29] adiroiban, so, I think it looks good in general, I don't like the naming for many things though :) [17:29] danilos: why do you think there will be performance issues? [17:30] adiroiban, well, we have to be careful about how many DB queries are executed and how many Storm objects we instantiate [17:33] is there an easy way of profiling sql query numbers and created storm objects ? [17:33] adiroiban, so, if we don't have everything we need exposed in the interface and we have to do something like go through all languages to get a total of something, we are a bit screwed (i.e. timeouts will happen) [17:34] yes, but those statistics are already generated [17:34] adiroiban, eg. imagine we need to get a total of translated and untranslated messages here; with this interface, we'd have to go through all languages and add them up [17:34] is just that there is a lot of duplicate code [17:34] adiroiban, right, I am just giving an example of what kind of things to watch out for [17:35] adiroiban, and that's the kind of thing we'd need to watch for when designing the interface [17:35] adiroiban, I am not pretending to be too smart this late in the day, though, so take that into consideration :) [17:39] adiroiban, anyway, if we can't think of anything else, I mostly have complaints about the naming :) [17:39] :) [17:39] adiroiban, not that I can come up with good names right now [17:40] I will try to rename then and will come back with a full implementation for distro and product series [17:41] adiroiban, sure, sounds good [17:42] adiroiban, the important distinction is that these are not languages, as I said above, but instead translations... perhaps using "translations" for the property name on the interface would be better [17:44] danilos: true... i was thinking language_translations [17:44] to denote that those translations are per language [17:45] adiroiban, right, I am struggling with coming up with good naming for this :) === gary_poster is now known as gary-lunch === matsubara-lunch is now known as matsubara === gary-lunch is now known as gary-poster === gary-poster is now known as gary_poster === salgado is now known as salgado-lunch === matsubara is now known as matsubara-afk === salgado-lunch is now known as salgado [22:24] * jpds has https://code.launchpad.net/~jpds/launchpad/fix_565345-mirror-prober-agent/+merge/23708 up for review. === jpds changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [noodles775,adiroiban(bug-564852),jpds] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === matsubara-afk is now known as matsubara === salgado is now known as salgado-afk