| === krkhan changed the topic of #launchpad-reviews to: On Call: abentley || reviewing: || queue: [krkhan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === sinzui changed the topic of #launchpad-reviews to: On Call: abentley || reviewing: || queue: [krkhan, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | abentley: you're not really currently on call, are you? | 05:48 |
|---|---|---|
| abentley | jtv, true, I'm not. | 05:49 |
| jtv | should you even be awake? :) | 05:49 |
| jtv | I'll just chuck myself on the queue. | 05:49 |
| abentley | jtv, yeah, it's only 12:50 | 05:49 |
| === jtv changed the topic of #launchpad-reviews to: On Call: - || reviewing: || queue: [krkhan, sinzui, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | ah, that's not so bad. | 05:49 |
| noodles775 | henninge: hi! Did you get a chance to look at my MP yesterday? | 08:16 |
| henninge | noodles775: I started but did not finish. Sorry. It's on my list this morning. | 08:17 |
| noodles775 | Ah oh... great, thanks henninge ! | 08:17 |
| === noodles775 changed the topic of #launchpad-reviews to: On Call: - || reviewing: || queue: [krkhan, sinzui, jtv, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === bigjools changed the topic of #launchpad-reviews to: On Call: - || reviewing: || queue: [krkhan, sinzui, jtv, noodles775, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| bigjools | noodles775: you happy with ui=rs for adding fields to forms? | 10:22 |
| noodles775 | bigjools: It's usually a chance to get a second set of eyes over any potential easy improvements, so best to request a ui review always. | 10:24 |
| bigjools | okay np | 10:24 |
| bigjools | noodles775: did you know the guitarist in The Offspring is called Noodles? :) | 10:25 |
| noodles775 | Nope! | 10:25 |
| StevenK | noodles775: http://en.wikipedia.org/wiki/Kevin_%22Noodles%22_Wasserman | 10:31 |
| henninge | noodles775: review sent | 10:32 |
| noodles775 | heh... looks like it wasn't for the same reason.. | 10:32 |
| noodles775 | Thanks henninge ! | 10:32 |
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb reviewing: - || queue: [krkhan, sinzui, jtv, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| gmb | Wow, looks like I'm going to have a busy morning. | 11:03 |
| * gmb grabs a drink before getting started | 11:03 | |
| bigjools | gmb: you removed my name from the queue ... you meany | 11:20 |
| gmb | bigjools, Hah. Why do I always manage to do that to you? | 11:21 |
| bigjools | subliminal | 11:21 |
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb reviewing: - || queue: [krkhan, sinzui, jtv, noodles775, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb reviewing: krkhan || queue: [sinzui, jtv, noodles775, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| gmb | Indeed | 11:21 |
| bigjools | let me know when you get to it anyway, I am making some, er tweaks :) | 11:21 |
| gmb | bigjools, Ok. May be a while... | 11:26 |
| bigjools | yeah I figured | 11:26 |
| gmb | krkhan, Which branch of yours do you want me to review? I see two in the queue. | 11:30 |
| gmb | export-Person-getBugSubscriberPackages and implement-Bug-findAttachments | 11:30 |
| gmb | krkhan, I'll just review the first one for now and come to the other one later. | 11:34 |
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb reviewing: krkhan || queue: [sinzui, jtv, noodles775, bigjools, krkhan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb reviewing: sinzui || queue: [sinzui, jtv, noodles775, bigjools, krkhan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| bigjools | gmb: ok it's all fixed, feel free to JFRI | 11:42 |
| gmb | bigjools, Righto. | 11:44 |
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb reviewing: jtv || queue: [noodles775, bigjools, krkhan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb reviewing: noodles775 || queue: [bigjools, krkhan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | Thanks gmb, I take it you found the right branch: https://code.edge.launchpad.net/~michael.nelson/launchpad/536700-present-packagebuilds-in-ppa-context-2/+merge/27594 | 12:11 |
| noodles775 | henninge: I've pushed a new rev with some changes, and discussed some other ones from your review on the MP... thanks! | 12:12 |
| gmb | noodles775, Yep. Just added the review now; MP page was just giving me the spinner of infinity for some reason. | 12:12 |
| noodles775 | Thanks gmb. | 12:13 |
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb reviewing: bigjools || queue: [krkhan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| gmb | bigjools, commercial-archive-bug-593636, right? | 12:16 |
| bigjools | yip | 12:16 |
| gmb | Roit | 12:16 |
| ajmitch | gmb: I've got a branch that I want to get in at some point, however it changes an area which currently has no tests. What would be the right thing to do there? | 12:18 |
| gmb | ajmitch, My immediate response would be that it would be very cool if you could add unit tests to cover your change. If you can cover the other stuff that isn't covered at the same time, cool, if not, file a bug and add an XXX in the unit test module that you create. | 12:19 |
| gmb | ajmitch, Out of interest, what are you touching that's not covered by tests? | 12:20 |
| ajmitch | scripts/ftpmaster-tools/sync-source.py | 12:20 |
| * wgrant watches the reviewers scatter. | 12:20 | |
| ajmitch | heh | 12:20 |
| gmb | bigjools, r=me | 12:20 |
| bigjools | lmao | 12:20 |
| gmb | ajmitch, AHAHAHAHAHA | 12:20 |
| gmb | *ahem* | 12:21 |
| bigjools | thanks gmb | 12:21 |
| ajmitch | also in another branch, adding webservice doctests for blueprints, which will be *fun* but doable :) | 12:21 |
| gmb | ajmitch, Not knowing the ftpmaster code at all, I don't know how hard it would be to add sane tests. | 12:21 |
| gmb | bigjools, do you know? | 12:21 |
| ajmitch | gmb: I love getting that sort of reaction... | 12:21 |
| bigjools | sync-source doesn't have tests because it's very very old | 12:21 |
| bigjools | and only usually gets changed by ubuntu guys | 12:21 |
| ajmitch | wgrant was just telling me that there's another sort-of-implementation of the same? | 12:22 |
| bigjools | and soyuz guys have plans for internal native syncing | 12:22 |
| ajmitch | in other words, here be dragons | 12:22 |
| bigjools | breathing fire | 12:22 |
| bigjools | and brimstone | 12:22 |
| * ajmitch should have picked something easy, like rewriting blueprints from scratch | 12:23 | |
| ajmitch | gmb: shall I put in a merge proposal for the 10-line diff I have, without tests, so that you or someone can pick at it? | 12:26 |
| gmb | ajmitch, Yeah, might as well. I might ask someone else to review it at the same time as me though so as to make sure I don't screw something up by approving it. | 12:27 |
| gmb | bigjools, Would you be willing to double check ajmitch's branch for me? | 12:27 |
| ajmitch | it's probably something that would need to be checked by someone using it | 12:27 |
| gmb | I don't know if bigjools counts. | 12:29 |
| * bigjools OTP, gimme some | 12:29 | |
| gmb | ajmitch, Stick it on the queue anyway. I'm going to grab some lunch in a minute so I'll take a look when I get back. | 12:29 |
| ajmitch | sure | 12:29 |
| gmb | bigjools, No rush, I'm going to graze for a bit | 12:29 |
| ajmitch | I'll be heading off to sleep in a few min | 12:29 |
| ajmitch | ok, merge proposal is in, I probably screwed it up somewhere :) | 12:34 |
| bigjools | gmb: yes I can check it | 12:50 |
| krkhan | gmb: hello | 12:59 |
| krkhan | gmb: i've posted the results of person tests in merge proposal comments. the code doesn't introduce any regression | 13:05 |
| gmb | krkhan, Ah, so I confused myself a bit (had you exported a property, the test would have broken, because we list the properties exported). | 13:09 |
| wgrant | gmb: I've got a couple to throw on the queue, if that's OK. | 13:09 |
| gmb | krkhan, So, you need to add a test in xx-person that shows you can call the methdo from the API | 13:09 |
| gmb | wgrant, Sure, go for it. | 13:09 |
| gmb | wgrant, In fact, I'll do it | 13:10 |
| gmb | Otherwise we'll stomp all over each other | 13:10 |
| wgrant | (https://code.edge.launchpad.net/~wgrant/launchpad/no-buildd-ogre-model/+merge/27410 and https://code.edge.launchpad.net/~wgrant/launchpad/bug-592935-hide-disabled-ppas/+merge/27411) | 13:10 |
| wgrant | Fair point. | 13:10 |
| krkhan | gmb: can i just write a unittest for the new method? or is doc test a requirement? | 13:10 |
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb reviewing: - || queue: [krkhan, wgrant, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| wgrant | Thanks. | 13:10 |
| gmb | np | 13:10 |
| gmb | krkhan, Doctest is easier because you have a lot of the work done for you in terms of setting up the webservice client (take a look at the existing test to see what I mean). | 13:11 |
| === matsubara-afk is now known as matsubara | ||
| mars | Hi gmb, just getting started for the day | 13:14 |
| krkhan | gmb: i had done unittests for other branch and wasnt familiar with doctests. but now that i'm going through xx-person.txt i can see what you mean. writing one now | 13:14 |
| gmb | mars, Hi. No worries :) | 13:14 |
| gmb | krkhan, Cool, thanks. I'll review your other branch now. | 13:14 |
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb reviewing: krkhan || queue: [krkhan, wgrant, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb reviewing: krkhan || queue: [wgrant, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === mars changed the topic of #launchpad-reviews to: on-call: gmb, mars reviewing: krkhan || queue: [krkhan, wgrant, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| mars | * gmb has changed the topic to: on-call: gmb reviewing: krkhan || queue: [wgrant, wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | 13:16 |
| === henninge_ is now known as henninge | ||
| gmb | krkhan, Did you have a pre-implementation call with anyone about your branch? | 13:37 |
| gmb | (i.e. your Bug.findAttachments() branch) | 13:38 |
| gmb | (or a pre-implementation chat, for that matter) | 13:39 |
| krkhan | gmb: i had discussed it with bryceh | 13:39 |
| krkhan | (my mentor for gsoc) | 13:39 |
| gmb | krkhan, Ok. | 13:39 |
| gmb | krkhan, I'm just thinking that there's a potential for this method to cause problems. | 13:39 |
| gmb | krkhan, What would happen if I ran findAttachments('foo') against a Bug with a 2GB memory dump attached to it? | 13:40 |
| === stub1 is now known as stub | ||
| === stub1 is now known as stub | ||
| === stub1 is now known as stub | ||
| krkhan | gmb: that's exactly what i discussed with bryce. should i limit the attachment sizes that are searched or search the text file in chunks? | 13:44 |
| gmb | krkhan, Hmm. It's an interesting problem. My first thought is that we should limit the size of the attachments that we search, since searching a 2GB file in chunks will still take ages and will probably time out. | 13:45 |
| gmb | krkhan, Also, my thought is that if someone decided to hit the API many, many times in sequence with this request on a large attachment they could down an appserver or two, but that may just be scaremongering. | 13:46 |
| gmb | krkhan, So, yes, I think limiting this to files under an entirely arbitrary size would be a sensible measure. Based on what we've seen in the past with filesizes, searching and timeouts I'd suggest < 50MB. Also, chunking might not be entirely terrible. | 13:47 |
| BjornT | gmb: your concerns are valid. gmb, krkhan: it would make more sense to try to use the db for this. then we get similar search behaviour as the rest of lp, and you will avoid certain issues | 13:48 |
| krkhan | gmb: okay. i'll add the attachment size limits and appropriate tests | 13:48 |
| === mrevell is now known as mrevell-lunch | ||
| krkhan | BjornT: but db doesn't have attachments | 13:49 |
| BjornT | memory and performance is a big piece of concern. another is that the search doesn't behave as the rest of the searches in lp. another is that the current implementation will fail if the attachment contains non-ascii characters | 13:49 |
| krkhan | BjornT: i will fix non-ascii problem in the next revision. but i'm confused as to how db can be used for attachment search | 13:51 |
| krkhan | we don't have full text indices for the attachments | 13:51 |
| BjornT | krkhan: well, i'm not sure exactly how we can use the db. but it would be worth talking to stub (our dba) about it, to see what he thinks. | 13:52 |
| BjornT | krkhan: another possible solution would be to get all the attachments and do the searching in the client | 13:52 |
| krkhan | BjornT: doing it on the client side is what i did earlier on. it takes ages to download-search through all and defeats the whole purpose. doing searches in chunks and having file size limits can make it memory friendly though | 13:54 |
| stub | Using the DB to search attachments is problematic. We would need to store the text of the attachment in the DB (say the first 20k). | 13:55 |
| krkhan | stub: that would still be problematic as the matching text might be in the last 20k? | 13:55 |
| stub | You are talking about only doing this for limited sized attachments - I chose 20k as the limit. | 13:56 |
| stub | But that was an arbitrary choice. | 13:56 |
| stub | It might be possible to maintain an full text index but not store the text in the db, but I haven't done that before. It would probably work fine. | 13:57 |
| krkhan | the issue is, as gmb said i'm targetting file size limits around 50mb. having 50mb per attachment in the db is going to kill | 13:57 |
| gmb | krkhan, The more I think about it, 50MB is far too big | 13:57 |
| stub | I don't know if you can feed a chunk that big to tsearch2 | 13:57 |
| gmb | We should be looking at far less than that. | 13:57 |
| gmb | krkhan, What do you forsee using this method for, specifically? What kind of attachments? | 13:57 |
| stub | Or using google to search the attachments if we don't mind only doing this for public ones. | 13:58 |
| krkhan | gmb: patches and dumps. so that "similar" bugs are automatically reported | 13:58 |
| krkhan | stub: would it be okay if a FTI is generated for every attachment that's added? | 13:59 |
| stub | Which are not really text, so full text search isn't necessarily the best fit. | 13:59 |
| krkhan | stub: yeah, that's what i was thinking | 13:59 |
| krkhan | to summarize: the problem is searching through text files on the server. the issue is being memory efficient. db searches aren't possible since data can't be in db and full text doesn't make sense for patches | 14:01 |
| krkhan | that only leaves having limits and doing in chunks. if 50 mb is large. we can have 5 mb. but that's arbitrary. if this approach is considered okay the limit itself can be adjusted later on | 14:02 |
| stub | In theory, we could add a 'fti' column to bugattachment and a GIST index so we can do full text searches. On upload, we would need to manually generate the tsvector to store in the fti column, since we can't use the trigger approach we use elsewhere. I don't think that would be a resource problem. I don't know if it a good idea though :) | 14:02 |
| BjornT | kiko: well, the issues are memory, performance (how long will it take to search through the files), and consistency with other searches in lp | 14:03 |
| stub | There will be a limit to how much text you can feed tsearch2 to generate a tsvector, which I forget. That will set your upper limit. | 14:03 |
| BjornT | krkhan: well, the issues are memory, performance (how long will it take to search through the files), and consistency with other searches in lp | 14:03 |
| krkhan | BjornT: i agree. consistency is a more troublesome issue since there isn't any other search in (afaik) that does something like this. | 14:05 |
| gmb | krkhan, BjornT, stub: Might I genially suggest we take this to the mailing list for discussion? Just in the interests of not getting stalled on this in the review channel. | 14:06 |
| stub | http://www.postgresql.org/docs/8.3/static/textsearch-limitations.html | 14:06 |
| gmb | (#launchpad-dev is equally valid) | 14:06 |
| stub | So it explodes if the length of the unique words + some overhead exceeds 1MB. | 14:07 |
| krkhan | stub: full text search will generate lots of false positives? "while" is a match regardless of the condition that the loop is using | 14:07 |
| krkhan | gmb: should i post this to the list | 14:08 |
| BjornT | gmb, krkhan: the mailing list sounds fine. instead of focusing on exactly how to implement findAttachments(), it would be good to explain what it will be used for. maybe there's a better solution to the problem | 14:08 |
| gmb | BjornT, krkhan, Agreed. I'll reject the branch and it can be resubmitted when a better solution is found. | 14:08 |
| stub | Consider changing the Librarian backend to some sort of database (instead of using the filesystem as the database) that allows you to index and search. | 14:08 |
| krkhan | BjornT, gmb: okay. that sounds fine | 14:09 |
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb, mars reviewing: wgrant || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| stub | krkhan: I don't think so. Using a GIST index for the lookup (GIN still has issues making it unsuitable for arbitrary user queries), you get false positives. PostgreSQL filters these by examining the stored tsvector (the contents of the 'fti' column) | 14:10 |
| stub | oh... | 14:10 |
| stub | Full text search is useful for searching for words in text. | 14:11 |
| stub | Sourcecode isn't really text. Punctuation is important, but text search strips that cruft out. | 14:11 |
| krkhan | stub: that's what i was saying. i'm not familiar with fti but i don't think it stores all the ++, --, <=s in the tsvectors | 14:12 |
| stub | PG is somewhat limited compared to, say, google - no phrase searches, no nearness. You do get boolean operations (foo AND bar OR NOT baz) | 14:12 |
| mars | gmb, since you know which of William's branches are in the queue, should I just move on to the activereviews stack? | 14:13 |
| krkhan | stub: apart from the fact that tokens are normalized to lexemes. so "failed" and "fails" match | 14:13 |
| gmb | mars, the one I'm *not* reviewing is https://code.edge.launchpad.net/~wgrant/launchpad/bug-592935-hide-disabled-ppas/+merge/27411 | 14:13 |
| stub | No. You feed it text, it generates a tsvector, which is a list of lexemes (words munged so 'pages' and 'page' generate the same lexeme) | 14:13 |
| stub | Yup | 14:13 |
| mars | gmb, ok | 14:14 |
| mars | abentley, ping, https://code.edge.launchpad.net/~abentley/launchpad/bad-branch-name/+merge/27549 is hosed: merge conflicts with devel. Do you still want it reviewed? | 14:14 |
| krkhan | stub: yes. so if i'm searching for code that contain "pages" code that contain "page" will match too. that is to say, the intended targets of patches and dumps aren't "texts" | 14:15 |
| mars | gmb, I'll take the other one then | 14:15 |
| === mars changed the topic of #launchpad-reviews to: on-call: gmb, mars reviewing: wgrant, wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| wgrant | mars, gmb: Thanks. | 14:16 |
| === mrevell-lunch is now known as mrevell | ||
| abentley | mars, sure. I'll just push the updated version... | 14:18 |
| krkhan | anyways. gmb, BjornT, stub: thanks for taking out time to review. will hopefully be back with a better approach soon | 14:18 |
| stub | krkhan: Yes. You can't really call that false positives though because it is by design. You can override how words get turned into lexemes so keep punctuation, no stemming etc. but we haven't done that. | 14:18 |
| krkhan | stub: i can do that overriding for a specific fti column in bug attachments? | 14:21 |
| krkhan | (table) | 14:22 |
| abentley | mars, updated. | 14:22 |
| mars | thanks abentley | 14:22 |
| wgrant | mars: Thanks. Can you ec2 land that, please? | 14:26 |
| mars | wgrant, certainly | 14:26 |
| gmb | wgrant, r=me on your other branch. I'll ec2 it for you. | 14:28 |
| wgrant | gmb: Great, thanks. | 14:29 |
| gmb | np | 14:29 |
| abentley | gmb, mars: could one of you review https://code.launchpad.net/~abentley/launchpad/fix-592319/+merge/27552 please? | 14:42 |
| mars | abentley, gmb, I'll take it | 14:43 |
| abentley | mars, thanks. | 14:43 |
| === mars changed the topic of #launchpad-reviews to: on-call: gmb, mars reviewing: wgrant, abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb, mars || reviewing: -, abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| krkhan | gmb: i've added doctest for my first branch: https://code.launchpad.net/~inspirated/launchpad/export-Person-getBugSubscriberPackages/+merge/27630 | 15:13 |
| === krkhan changed the topic of #launchpad-reviews to: on-call: gmb, mars || reviewing: -, abentley || queue: [krkhan] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| gmb | krkhan, Thanks, will take a look in a moment. No need to add yourself to the queue again; it comes straight to my inbox once I've reviewed your branch once. | 15:14 |
| mars | abentley, done | 15:15 |
| === gmb changed the topic of #launchpad-reviews to: on-call: gmb, mars || reviewing: -, abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| mars | I so want an IRC bot | 15:15 |
| krkhan | gmb: ah okay. thanks | 15:15 |
| abentley | mars, thanks. | 15:16 |
| mars | gmb, shall I take wgrant's third branch? https://code.edge.launchpad.net/~wgrant/launchpad/bug-592914-recipe-distroseries-order/+merge/27415 | 15:17 |
| gmb | mars, Please; I'm OTP at the moment. | 15:20 |
| mars | done | 15:21 |
| === mars changed the topic of #launchpad-reviews to: on-call: gmb, mars || reviewing: -, wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| abentley | mars, we're not reporting the fact that the branch doesn't exist. We're reporting the fact that it's not stored on launchpad (and may not exist). | 15:21 |
| mars | abentley, oh, so the Exception itself is clearly named in the Launchpad domain, but the error text is clear in the wider Launchpad user's domain | 15:22 |
| abentley | mars, you agreed to review my other request: https://code.launchpad.net/~abentley/launchpad/fix-592319/+merge/27552 | 15:22 |
| mars | abentley, gah, ok | 15:23 |
| mars | abentley, I'll do that first then | 15:23 |
| gmb | krkhan, Your test tests that the method works, but not that's it's called through the webservice. If you look elsewhere in xx-person.txt you'll see webservice getters being tested using webservice.named_get(). That's what you need to use. | 15:38 |
| mars | abentley, done, r=mars | 15:40 |
| abentley | mars, thanks. | 15:40 |
| gmb | krkhan, So something like | 15:40 |
| krkhan | gmb: okay. going to do that now | 15:40 |
| gmb | krkhan, Ah, cool. | 15:40 |
| gmb | krkhan, Feel free to ping me if you need more info. | 15:40 |
| abentley | mars, I'd be happy to change that message to "foo is not a branch on Launchpad" or something similar. Would that be better? | 15:49 |
| mars | abentley, how often do you expect users to accidentally enter the name of a branch that is not on launchpad, vs. how often do you expect them to misspell an existing LP branch name? | 15:50 |
| mars | common or uncommon? | 15:50 |
| abentley | I think it will be more common to misspell a branch location that is on Launchpad, than to enter one that is not on Launchpad. The latter is a newbie mistake. | 15:53 |
| mars | ok. Thats' why I think "No such branch" sounds clear - it addresses the most common mistake, but still addresses the rare one. | 15:54 |
| abentley | mars, I don't think it addresses the case where the branch exists, but is not on Launchpad. | 15:55 |
| mars | abentley, in that case, I'd suggest going with "No such branch" or "Unknown branch". I like the former for the stated reasons. Up to you. | 15:55 |
| abentley | mars, cool. | 15:57 |
| mars | Well, if you enter a bad branch, "No such branch: my-site/foo" and "Unknown branch: my-site/foo" are just as newbie confusing. | 15:57 |
| mars | But for a LP user, "No such branch: lp:~foo" is quite clear language. "Unknown branch: lp~foo" is just a bit more vague, that's all. | 15:58 |
| abentley | mars, and why is "foo is not a branch on Launchpad" not less confusing? | 15:59 |
| mars | abentley, reading it again, you are right, it is fine as well. A bit more verbose. Might sound better. | 16:02 |
| mars | abentley, sorry for the trouble, I'm just trying to find a balance between brusque messaging and clear user text. | 16:02 |
| abentley | mars, I know, this stuff is never easy. There's often tension between precision and clarity. | 16:03 |
| mars | abentley, I trust your judgement. Go with what you think fits. | 16:04 |
| mars | gmb, afk for a bit | 16:08 |
| === mars changed the topic of #launchpad-reviews to: on-call: gmb, mars[afk] || reviewing: -, wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| gmb | mars, Ok. | 16:10 |
| === salgado is now known as salgado-afk | ||
| * gmb knocks off for the evening | 17:00 | |
| === gmb changed the topic of #launchpad-reviews to: on-call: mars[afk] || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === kiko is now known as kiko-fud | ||
| === mars changed the topic of #launchpad-reviews to: on-call: mars || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| krkhan | gmb: updated my branch to use webservice.get | 18:20 |
| === deryck is now known as deryck[lunch] | ||
| === mars changed the topic of #launchpad-reviews to: on-call: mars || reviewing: ajmitch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| deryck[lunch] | hi mars. I'll add my branch to the queue but will be away eating, so if someone needs to jump me in line, they certainly should. | 18:45 |
| === deryck[lunch] changed the topic of #launchpad-reviews to: on-call: mars || reviewing: ajmitch || queue: [deryck] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| mars | deryck[lunch], sure, thanks | 18:46 |
| joey | mars: there you go | 18:59 |
| joey | mars: folks just need to part and then join | 18:59 |
| mars | joey, erm, not sure if that works | 19:00 |
| mars | joey, was thinking "On-call reviewers should have voice, it would be easier to tell them apart" | 19:00 |
| mars | joey, if everyone who logs in has voice, that doesn't work as well | 19:00 |
| joey | mars: well, you have already done that I think with asking chanserv for it but it would be a manual process | 19:01 |
| joey | mars: now just ask for voice | 19:03 |
| mars | joey, perfect! Thanks | 19:03 |
| joey | there you go | 19:03 |
| joey | glad I could help | 19:03 |
| krkhan | BjornT: ping | 19:07 |
| === matsubara is now known as matsubara-afk | ||
| === deryck[lunch] is now known as deryck | ||
| === mars changed the topic of #launchpad-reviews to: on-call: mars || reviewing: deryck || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === kiko-fud is now known as kiko | ||
| ajmitch | mars: thanks for checking over that fakesyncs branch. the lack of tests is certainly a problem, but I'm not sure how they could be added for that | 20:33 |
| mars | ajmitch, did one of the Soyuz team offer to smoke-test it on dogfood? | 20:33 |
| ajmitch | nope, I'd put it up there for gmb/bigjools to look at earlier, when I was awake & in-channel :) | 20:34 |
| ajmitch | it's most likely not ready for landing just yet until it can be checked over with some real data | 20:35 |
| ajmitch | plus I need to put in the contributors agreement :) | 20:35 |
| mars | ok, maybe explicitly add bigjools to the list, or drop him an email. | 20:35 |
| mars | ah, that too, yes | 20:35 |
| mars | add bigjools to the list of requested reviewers | 20:35 |
| ajmitch | besides that, I had a brief chat with an ubuntu archive admin to see if the idea was sane, but no real pre-impl call | 20:36 |
| mars | ok, so the policy is good. bigjools will be able to double-check the implementation | 20:36 |
| === matsubara-afk is now known as matsubara | ||
| === adiroiban changed the topic of #launchpad-reviews to: on-call: mars || reviewing: deryck || queue: [adiroiban(bug-590899 pre-impl,ui)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| mars | adiroiban, I can take that now | 21:32 |
| adiroiban | mars: I have attached a mockup to the bug | 21:32 |
| adiroiban | https://bugs.edge.launchpad.net/rosetta/+bug/590899 | 21:32 |
| _mup_ | Bug #590899: Non human readable plural expression in language web view <ui> <Launchpad Translations:Triaged by adiroiban> <https://launchpad.net/bugs/590899> | 21:32 |
| adiroiban | it should be simple | 21:32 |
| === mars changed the topic of #launchpad-reviews to: on-call: mars || reviewing: adiroban || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adiroiban | but the bug needs an UI pre-impl talk | 21:32 |
| adiroiban | http://launchpadlibrarian.net/50391137/pofile-details.png | 21:33 |
| mars | rockstar, ^ do you want to take that? or should we leave it for one of the UI mentats? | 21:33 |
| adiroiban | we need to decide where and how to present the following text „Plural formula: n==1 ? 0 : (n==0 || (n%100 > 0 && n%100 < 20)) ? 1 : 2” | 21:33 |
| rockstar | mars, I'm on leave all week. I ain't touchin' it. | 21:33 |
| mars | rockstar, ah! | 21:34 |
| mars | sinzui, ping? | 21:34 |
| sinzui | hi mars | 21:34 |
| mars | hi sinzui - any advice for who should pick up adiroiban's UI review? | 21:35 |
| sinzui | Me or noodles | 21:35 |
| adiroiban | sinzui: no hurry to do this review. I was thinking that mars can do it as part of his path to become a UI reviewer | 21:37 |
| sinzui | adiroiban, https://code.edge.launchpad.net/~adiroiban/launchpad/512735/+merge/23759 ? | 21:37 |
| adiroiban | sinzui: no | 21:37 |
| adiroiban | this is an pre-implementation talk | 21:38 |
| adiroiban | https://bugs.edge.launchpad.net/rosetta/+bug/590899 | 21:38 |
| _mup_ | Bug #590899: Non human readable plural expression in language web view <ui> <Launchpad Translations:Triaged by adiroiban> <https://launchpad.net/bugs/590899> | 21:38 |
| mars | adiroiban, that's been over a year now, and I don't plan to work on it any time soon :) | 21:38 |
| adiroiban | henninge ask to have an an UI pre-impl talk | 21:38 |
| sinzui | adiroiban, sorry, I was confused | 21:38 |
| adiroiban | and this is why I am bringing this here | 21:39 |
| mars | makes sense | 21:39 |
| sinzui | adiroiban, do you wan me to send to my remarks or do you want to talk about it over irc or skype? | 21:39 |
| adiroiban | sinzui: http://launchpadlibrarian.net/50391137/pofile-details.png | 21:39 |
| sinzui | I am looking at that now. | 21:40 |
| === mars changed the topic of #launchpad-reviews to: on-call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adiroiban | pick the communication channel that you think it is more appropriate | 21:40 |
| sinzui | adiroiban, I will send you my notes, then we can discuss them over irc/skype if we need more information | 21:41 |
| adiroiban | sinzui: ok. the mockup source file is here: http://bazaar.launchpad.net/~launchpad-dev/launchpad/rosetta-pofile-translate-mockups/annotate/head:/pofile-details.bmml | 21:42 |
| sinzui | fab | 21:42 |
| mars | thanks sinzui, adiroiban | 21:43 |
| === mars changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === matsubara is now known as matsubara-afk | ||
| bryceh | on-call: - || reviewing: - || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | 23:30 |
| === bryceh changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| bryceh | dah | 23:30 |
| wgrant | mars: Oops, I didn't know that attrgetter could handle a second level like that, so I deliberately avoided it. | 23:32 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
