| === 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 | ||
| jtv | No volunteers for a review today? | 10:41 |
|---|---|---|
| === henninge_ is now known as henninge | ||
| henninge | jtv: Let's swap: https://code.edge.launchpad.net/~henninge/launchpad/bug-516736-templatename/+merge/23659 | 12:15 |
| jtv | henninge: OK! | 12:16 |
| jtv | henninge: s/\<otemplate/potemplate/ | 12:20 |
| jtv | henninge: also, it may help to introduce a variable in the last paragraph of the diff: | 12:24 |
| jtv | potemplate = self.context.potemplate | 12:24 |
| jtv | if potemplate is None: | 12:24 |
| jtv | # ... | 12:24 |
| jtv | else: | 12:24 |
| jtv | # Do stuff with potemplate, not self.context.potemplate | 12:24 |
| henninge | jtv: yeah, nice idea. Although one line lost, one line gained... | 12:25 |
| henninge | jtv: how did you "tighten the check a bit more" ? | 12:32 |
| jtv | 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 |
| henninge | oic | 12:34 |
| jtv | henninge: anyway, I'm approving yours. | 12:34 |
| henninge | jtv: is it this? | 12:34 |
| henninge | 144- if pluralforms_count != len(forms): | 12:35 |
| henninge | 145+ found_forms = sorted(forms.keys()) | 12:35 |
| henninge | 146+ if found_forms != range(expected_forms): | 12:35 |
| jtv | henninge: right | 12:35 |
| henninge | cool | 12:35 |
| henninge | jtv: thanks | 12:35 |
| jtv | 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:35 |
| henninge | jtv: I just have two small things, I'll mention them in the review. r=me | 12:36 |
| jtv | henninge: dankeschöen | 12:37 |
| jtv | ahem | 12:37 |
| henninge | doppelt gemoppelt | 12:37 |
| jtv | ön | 12:37 |
| henninge | ;-) | 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 | ||
| henninge | jtv: review sent | 12:44 |
| jtv | thanks | 12:44 |
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| sinzui | BjornT, ping | 15:28 |
| BjornT | hi sinzui | 15:29 |
| sinzui | 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:30 |
| BjornT | 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:32 |
| sinzui | 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 |
| BjornT | 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 |
| sinzui | I will choose a different name since this is really about a portlet behaviour | 15:33 |
| BjornT | sinzui: ok, cool | 15:34 |
| === 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 | ||
| adiroiban | 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 |
| mup | Bug #512735: At first diplay only preferred languages for a POFile from a POFileSubset <tech-debt> <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/512735> | 16:46 |
| danilos | adiroiban, sure | 17:03 |
| danilos | adiroiban, should I look at the branch? | 17:03 |
| adiroiban | yes | 17:05 |
| === matsubara is now known as matsubara-lunch | ||
| adiroiban | danilos: there is an IHasLangaugesWithTranslations interface | 17:05 |
| danilos | adiroiban, right, looking | 17:06 |
| adiroiban | and an implementation of language-statistics view | 17:06 |
| danilos | adiroiban, so, I am not sure this is quite enough; this is definitely a step in right direction though | 17:06 |
| adiroiban | danilos: what is not enough? the fields from the interface? | 17:06 |
| danilos | adiroiban, yeah, I am not sure that's completely enough | 17:07 |
| danilos | adiroiban, still looking though :) | 17:10 |
| danilos | adiroiban, the biggest concern is performance | 17:10 |
| danilos | adiroiban, also, I suspect you based the generic languages-translations-statistics.pt on one of distroseries or productseries templates | 17:11 |
| danilos | adiroiban, it would have been much easier to read that if you moved one of them to it | 17:11 |
| danilos | adiroiban, also, has_translations and languages_statistics should probably be @properties | 17:12 |
| adiroiban | danilos: true. my bad for not moving one of those files. And yes, those should be properties | 17:19 |
| adiroiban | 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:22 |
| adiroiban | and then see what other fields are required by sourcepakage and potemplate and add them later | 17:23 |
| danilos | 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:24 |
| danilos | adiroiban, btw, LanguageStatisticsView should definitely not be in browser/language.py :) | 17:25 |
| adiroiban | danilos: agree, I was not sure where to put it. Should I add it in a new file? | 17:26 |
| danilos | adiroiban, sure, please do... it's also not really "language_statistics", it's "per-language statistics" | 17:26 |
| danilos | adiroiban, so, I think it looks good in general, I don't like the naming for many things though :) | 17:29 |
| adiroiban | danilos: why do you think there will be performance issues? | 17:29 |
| danilos | adiroiban, well, we have to be careful about how many DB queries are executed and how many Storm objects we instantiate | 17:30 |
| adiroiban | is there an easy way of profiling sql query numbers and created storm objects ? | 17:33 |
| danilos | 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:33 |
| adiroiban | yes, but those statistics are already generated | 17:34 |
| danilos | 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 |
| adiroiban | is just that there is a lot of duplicate code | 17:34 |
| danilos | adiroiban, right, I am just giving an example of what kind of things to watch out for | 17:34 |
| danilos | adiroiban, and that's the kind of thing we'd need to watch for when designing the interface | 17:35 |
| danilos | adiroiban, I am not pretending to be too smart this late in the day, though, so take that into consideration :) | 17:35 |
| danilos | adiroiban, anyway, if we can't think of anything else, I mostly have complaints about the naming :) | 17:39 |
| adiroiban | :) | 17:39 |
| danilos | adiroiban, not that I can come up with good names right now | 17:39 |
| adiroiban | I will try to rename then and will come back with a full implementation for distro and product series | 17:40 |
| danilos | adiroiban, sure, sounds good | 17:41 |
| danilos | 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:42 |
| adiroiban | danilos: true... i was thinking language_translations | 17:44 |
| adiroiban | to denote that those translations are per language | 17:44 |
| danilos | adiroiban, right, I am struggling with coming up with good naming for this :) | 17:45 |
| === 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 | ||
| * jpds has https://code.launchpad.net/~jpds/launchpad/fix_565345-mirror-prober-agent/+merge/23708 up for review. | 22:24 | |
| === 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!