Code review comment for lp:~posulliv/drizzle/bug337038-dec-trunc-should-error

Revision history for this message
Jay Pipes (jaypipes) wrote :

The patch looks good, but I'd like to see some more documentation in the test cases for decimal. Padraig, do you think you could:

a) Separate out the multi-value INSERT statements in the test case into single INSERT statements? This makes it easy to track down which, if any, of the VALUES may trigger a warning or error.

b) Put a comment above each SELECT and INSERT you modified in the test case to indicate *why* that specific value was used in the test case. As it is now, numbers seem to be fairly randomly chosen. Would be good to see the test case be more explicit about the edge cases it intends to validate...

Other than that, everything looks great. :)

-jay

review: Needs Resubmitting

« Back to merge proposal