WebReflection / dblite

sqlite for node.js without gyp problems
MIT License
209 stars 34 forks source link

lastRowID() doesn't work in -header mode #13

Closed bartve closed 10 years ago

bartve commented 10 years ago

lastRowID() doesn't handle -header mode yet.

WebReflection commented 10 years ago

lastRowID is a special case that returns anything but the last inserted row id. Can I ask you which problem are you trying to solve? Is it about parsing? in such case maybe the parseInt could be probably used as default … or something else? Thanks

WebReflection commented 10 years ago

uh … never mind, it's about the mode. I'll check that, sorry for the confusion (however, having it as integer is probably better too)

WebReflection commented 10 years ago

latest has tests in place and should be fixed. Feel free to re-open if you have problems, thanks.

bartve commented 10 years ago

Thanks for fixing! Wouldn't this simpler code work just the same? Just curious why you've made your code so elaborate with type checks and hasOwnProperty etc.

var row = result[0], id;
// You can iterate both an array and object the same way, 
// with objects there's no way of telling the order, but it's always 1 row, so it doesn't matter.
for (var key in row) {
    id = row[key]; // Could do a pareseInt(row[key],10)
    break; // Not even needed as there's a LIMIT 1 in the query.
}
(callback || log).call(self, id);

Not sure about the parseInt as the ROWID is a signed 64 bit integer (http://www.sqlite.org/autoinc.html) and Javascript doesn't seem to handle the full range of that http://stackoverflow.com/q/9643626 Or do we only need a 32 bit Javascript int as in SQLite it's signed and usually only positive ROWIDs are used? I'm not too familiar in this territory.

WebReflection commented 10 years ago

that works for me and for you, then somebody might have the brilliant idea to enrich the Array.prototype that might show up in there instead of 0 or somebody else might have change the Object.prototype without forcing non enumerability. dblite would like to ensure consistent behavior across environments, node.js versions, etc etc, and since this is usually a one shot operation I think my change is safe enough and not so bad in terms of RAM or CPU cycles. About the parseInt, yes, I've realized I should not do that in here.

bartve commented 10 years ago

Thanks for the comments. It wasn't criticism, just interested in why the fluffy code.

WebReflection commented 10 years ago

I could have gone smarter with a row[Object.keys(row).shift()] more explicit, safe, and concise than yours but that would have created a pointless Array for the GC and I was happy to avoid it ^_^

might be premature or too extreme measure though, I might update later on. Right now it works which is what matters ;-)