[05:48] <StevenK> wgrant: So, JavaScript and microservices?
[05:48] <wgrant> StevenK: Have you repushed the JS branch?
[05:50] <StevenK> wgrant: Yeah, the .set() changes have been up since I started on destroying PF, I've just forgotten to ping
[05:52] <wgrant> Ah
[05:55] <wgrant> StevenK: Have you tested person names with special characters such as .?
[05:55] <wgrant> And some of that code still uses appendChild excessively, which makes it pretty unreadable and slow when compared to code that creates the complex structure in a single Y.Node.create
[05:56] <wgrant> (eg. the bit under draft [05:57] <wgrant> I think it creates a <tr><td></td><td><td><div><span></span></div></div></td></tr>, but that would be much clearer if created as a single object.
[05:57] <StevenK> wgrant: Converting that bit to using .set() almost forced my brain out of my ears to begin with
[05:58] <wgrant> There were at least two mistakes in there.
[05:58] <StevenK> wgrant: One two many <td>'s, and there's a missing <div>
[05:58] <StevenK> One too, even
[05:58] <wgrant> StevenK: You only need to set two IDs, which should be pretty trivial.
[05:59] <StevenK> wgrant: Sure, but I want the outer div
[05:59] <wgrant> node.one('tr > td > div') or use a class
[05:59] <StevenK> Hm, yeah
[06:00] <StevenK> wgrant: So newrow = Y.Node.create('{everything}'); and then two lines to call .one().set() ?
[06:01] <wgrant> StevenK: I think that's a lot clearer, yeah
[06:01] <wgrant> You can see the structure
[06:01] <wgrant> And easily see what dynamic bits you're adding
[06:01] <wgrant> And it's faster.
[06:02] <StevenK> wgrant: Let me polish off dealing with this e-mail, and I'll change draft [06:24] <StevenK> wgrant: http://pastebin.ubuntu.com/6095769/
[06:28] <wgrant> StevenK: That looks superior.
[06:31] <StevenK> Hmmm, newone.one('tr') is null :-(
[06:36] <wgrant> StevenK: Isn't that the node itself?
[06:36] <wgrant> So finding a tr child is not going to be terribly effective.
[06:36] <wgrant> s/child/descendant/
[06:36] <StevenK> Yeah, I came to that realisation about 45 seconds ago
[06:43] <StevenK> wgrant: Still not sure what to do about dots in names
[06:46] <wgrant> StevenK: dots are fine in IDs, but then you're using CSS selector syntax to find them, which will fail.
[06:46] <StevenK> So I should switch to person.id ?
[06:47] <wgrant> We don't usually expose person IDs.
[06:47] <wgrant> That's one possibility, but you could also just not use Node.one() to find them
[06:48] <StevenK> And do what instead?
[06:49] <wgrant> Fetch them directly by ID, rather than a CSS selector under a node
[06:49] <wgrant> Recall that IDs are globally unique.
[06:49] <stub> No quoting or escaping mechanism?
[06:49] <wgrant> One could also quote them, but it's not necessary here.
[06:50] <StevenK> Hmm
[07:18] <Ursinha> wgrant, should I move all checks in publishRosettaTranslations to RosettaTranslationsUpload or moving away the part that creates the job is enough?
[07:19] <wgrant> Ursinha: I think it makes sense to move the whole lot, for consistency.
[07:19] <Ursinha> wgrant, right.
[07:20] <wgrant> The custom upload types are fundamentally different in that rosetta translations don't get extracted, but we should still keep the code as similar as is practical.
[07:24] <Ursinha> wgrant, so I could move it to a _publishRosettaCustom or something, should have the same effect and will keep publishRosettaTranslations consistent with the rest
[07:24] <Ursinha> instead of moving the checks to RosettaTranslationsCustom, I mean
[07:29] <Ursinha> in the end it's your call anyway
[07:29] <Ursinha> :)
[07:33] <wgrant> Ursinha: Sorry, let me see.
[07:36] <wgrant> Ursinha: Ah, I see. We obviously can't use _publishCustom for this, as that assumes we're extracting a file. So I'd just move the contents of publishRosettaTranslations to new function in lp.archivepublisher.rosetta_translations and call that directly in publishRosettaTranslations.
[07:36] <wgrant> Since we don't need the extra tempfile/cleanup behavior that _publishCustom provides
[07:38] <Ursinha> wgrant, I thought we could avoid having to import a lot of things to do the checks in lp.archivepublisher.rosetta_translations when we already have it all in queue.py :) I was suggesting move the checks to a _publishRosettaCustom instead
[07:39] <wgrant> Ursinha: I think it's probably still slightly nicer to have it encapsulated in the other module like the rest, rather than inline in queue.py.
[07:39] <wgrant> But I don't care too much.
[07:40] <Ursinha> because _publishCustom prepares and runs the process_* methods, so I thought it would make sense to have the checks before process_rosetta_translations as well
[07:40] <wgrant> _publishCustom doesn't do sanity checks, though
[07:40] <Ursinha> indeed
[07:41] <wgrant> It just prepares the temporary file in a generic way, without doing anything type-specific.
[07:41] <Ursinha> yeah, the sanity checks are all in CustomUpload.process...
[07:41] <Ursinha> okay, I'll move it all :)
[07:42] <wgrant> Sounds good.
[09:19] <Ursinha> wgrant, should I also add tests for this change in lp.soyuz.tests.test_distroseriesqueue_rosetta_translations.py?
[09:26] <cjwatson> IIRC the archivepublisher tests are unit tests and the soyuz.tests.test_distroseriesqueue tests are kind of integration tests.
[09:40] <Ursinha> right
[10:21] <wgrant> Ursinha: I believe cjwatson is correct. The tests for the other custom uploads aren't a completely terrible example, IIRC
[10:22] <cjwatson> Yeah, I think I beat them into shape in the last big refactoring
[10:22] <cjwatson> Looks like I converted them from doctests
[10:23] <wgrant> Yeah
[10:23] <wgrant> They used to be one or two doctests each