search

jplindstrom / p5-DBIx-Class-BatchUpdate

Perl module to update batches of DBIC rows with as few queries as possible
0 stars 1 forks source link

Don't use "magic values" for differentiation #1

Closed ribasushi closed 8 years ago

ribasushi commented 8 years ago

This is a really fragile way of approaching this. Imagine someone writing an unpacked version of everything on CPAN to a database and comes across the source of your module. OOOOOPS.

Instead use a proper serializer which will account for undefs, and any other weird shit out there (however do not use the module I linked - it's internal with no API guarantees)

ribasushi commented 8 years ago

P.S. good idea otherwise, ++ ;)

jplindstrom commented 8 years ago

Fair point. The difference between inhouse code and "software".

Will fix tomorrow.

ribasushi commented 8 years ago

Your current fix is fine, but from a performance perspective just producing a raw Storable::nfreeze across the entire bag of values will be much cheaper than what you implemented.

Just a followup thought, functionality-wise the problem is resolved.

ribasushi commented 8 years ago

Actually I misread - what is $_ expected to be set to here?

jplindstrom commented 8 years ago

Good catch, that's bad form. It happens to work in this particular case so the test passed :)

But since I'll have to touch it again I'll just replace it with nfreeze. Initially I wouldn't trust its representation to be stable, but now I see it's specifically mentioned in the docs.

ribasushi commented 8 years ago

Right, it's guaranteed stable on the same version of Storable.pm as long as you set local $Storable::canonical = 1;.

Across various perl/Storable versions the stability is a different story, but that's not a concern in your case.

Cheers!