Code review comment for lp:~rockstar/launchpad/no-duplicate-recipe-names

The db patch itself is fine, I do have some objections on the test, though.

Not sure if this is just a temporary test that you plan to replace, or not. If it is temporary, you should add an XXX saying so.

The issue is that you shouldn't rely on IntegrityError. I think the test you wrote shows why. First of all, the error doesn't happen when you call the method. It happens later, when you call store.flush(), which makes it really hard to see exactly what went wrong.

The second thing is that if you rely on IntegrityError, all that you know is that something went wrong. Your test doesn't show that it's the constraint that you added that got triggered, it could just as well be something else that went wrong. Catching IntegrityError is a bit like catching Exception. It's something you should do only if you need to make the code fail more gracefully, like logging an OOPS.

Third, if you trigger an Integrity error, the transaction gets hosed. So if you try to read something more from the db, you will get an error.

DB constraints are mainly a safety net for broken code, and ideally should never be triggered. It's better to look before you leap in this case.

On a side note, I don't understand the comment "The metadata supplied when a SourcePackageRecipe is created is present on the new object." It seems to relate neither to the test, nor to the next line.

So, +1 for the patch, -1 for the test.