search

IreneKnapp / direct-sqlite

MIT License
35 stars 54 forks source link

possible packaging of extension loading #46

Closed mwotton closed 10 years ago

mwotton commented 10 years ago

Not sure of what your preferred packaging of this could be - this is a relatively low-level binding.

IreneKnapp commented 10 years ago

Hmm, this looks decent. Can we get the result as an Error (use toResult) instead of an Int? I'm of two minds about whether to coalesce these into one function, taking a boolean... I think it's probably fine as two, the way you have it.

mwotton commented 10 years ago

Yes, that makes sense. I can patch that up when I get back - currently sitting in da nang with a flat tyre on the bike :)

Sent from my iPhone

On 19 Jun 2014, at 11:33 am, Irene Knapp notifications@github.com wrote:

Hmm, this looks decent. Can we get the result as an Error (use toResult) instead of an Int? I'm of two minds about whether to coalesce these into one function, taking a boolean... I think it's probably fine as two, the way you have it.

— Reply to this email directly or view it on GitHub https://github.com/IreneKnapp/direct-sqlite/pull/46#issuecomment-46522761.

mwotton commented 10 years ago

@IreneKnapp is that ok?

IreneKnapp commented 10 years ago

Oh, sorry! Yes, that's good, and thank you for doing this work. I'll release to Hackage most likely tomorrow.

IreneKnapp commented 10 years ago

Yeah, cutting to one entry point seems reasonable, but I'm not the one who has production code that will need changing. :)

I mean, I think you want the return type as IO (Either Error ()), rather than IO Int, but other than that.

nurpax commented 10 years ago

AFAICT this was never released to Hackage, so I'm hoping no hurt feelings if this changes.

Yep, the return type is already Either Error () in master, that's what it needs to be even if I merge this to a single function.

@mwotton I hope you had a great time in Da Nang. :)

mwotton commented 10 years ago

I'm not too fussed - matter of a couple lines in some of my code.

On Thu, Oct 2, 2014 at 4:03 AM, Janne Hellsten notifications@github.com wrote:

AFAICT this was never released to Hackage, so I'm hoping no hurt feelings if this changes.

Yep, the return type is already Either Error () in master, that's what it needs to be even if I merge this to a single function.

— Reply to this email directly or view it on GitHub https://github.com/IreneKnapp/direct-sqlite/pull/46#issuecomment-57538024 .

A UNIX signature isn't a return address, it's the ASCII equivalent of a black velvet clown painting. It's a rectangle of carets surrounding a quote from a literary giant of weeniedom like Heinlein or Dr. Who. -- Chris Maeda

nurpax commented 9 years ago

I prefer the two entry points - "enableLoadExtension False" just reads badly to me.

@mwotton Yeah, not the best name. I'm thinking to use setLoadExtensionEnabled. Bunch of other APIs in various frameworks seem to use a similar name.

I dislike having one entry point for True, one for False. For example, if the enable/disable value was coming from a config file, one would need to do something like this:

enabled <- getConfigValue "loadExtension"
if enabled then 
  enableExtensionLoad conn
else
  disableExtensionLoad conn

whereas with the bool param it would just be getConfigValue "loadExtension" >>= setExtensionLoadEnabled conn.

nurpax commented 9 years ago

This is now in direct-sqlite-2.3.14 on Hackage.