Code review comment for lp:~abentley/launchpad/package-build-recipe

Revision history for this message
Paul Hummer (rockstar) wrote :

<rockstar> abentley, why do you catch ProgrammingError and then raise without an argument?
<abentley> rockstar, I want ProgrammingErrors to propagate normally, because it's confusing if that exception handler tries to swallow them, and it masks the original cause.
<rockstar> abentley, ah, is this the problem you were bumping into this morning?
<abentley> rockstar, I raise without an argument because I want to see the traceback from where it was originally raised.
<abentley> rockstar, yes.
<rockstar> abentley, oka.
<rockstar> abentley, I wonder if a comment to that effect might be good.
<rockstar> abentley, actually, no, probably not.
<rockstar> abentley, I don't see any tests for lfaUrl, log_url, or upload_log_url. Are those abstracted somewhere?
<abentley> rockstar, I shouldn't have to test those, because they're provided by packagebuild.
<abentley> or buildfarmjob for log_url.
<rockstar> abentley, okay, lemme actually fetch the branch. The diff seems misleading then.
<rockstar> abentley, it looks like lfaURL is part of SourcePackageRecipeBuild
<abentley> rockstar, Yes, there are tests for that one.
<abentley> rockstar, that's basically a view helper.
<rockstar> abentley, can you point me to the tests for it?
<abentley> rockstar, it makes +files/upload-foo.log work.
<rockstar> abentley, okay, so it's not unit tested, but the functionality is being tested.
<abentley> rockstar, right.
<rockstar> abentley, I guess that's okay, although I'd be more comfortable with a unit test as well.
<rockstar> I leave that to your discretion.
<abentley> rockstar, I only added it because there were failing tests.

review: Approve (code)

« Back to merge proposal