JuliaDatabases / PostgreSQL.jl

DEPRECATED: use LibPQ.jl instead
https://github.com/invenia/LibPQ.jl
Other
60 stars 39 forks source link

Added dispatch to connect to connect with just DSN #11

Closed cuevasclemente closed 9 years ago

cuevasclemente commented 9 years ago

Hey,

The main point here was to add a method to connect that allows one to connect with just a DSN. A test was added for this and some print statements were added to the test.

Some of the other change addresses naming conflict with dataframes similar to what was discussed in https://github.com/iamed2/PostgreSQL.jl/issues/9. I think these name conflicts happen because when you use using ModuleName, it will require ModuleName and if ModuleName == modulename.jl, it causes an import. I think this may be a bug in Julia, but it is easy enough to address with a similar strategy as what was used in https://github.com/iamed2/PostgreSQL.jl/pull/10

Tests passed for me on my local box with a local version of PostgreSQL running.

iamed2 commented 9 years ago

The problem with connecting with only a DSN is that it overrides the method which allows connecting with only a host. I think the DSN connect method might have to have a different name. Alternatively, There could be a method connect(::Type{Postgres}; dsn::String="") which would not conflict.

I'll have to fix the issues with nightly compatibility and add Julia 0.3 tests on Travis so we don't have to rely on local testing.

cuevasclemente commented 9 years ago

I had not forseen this. I do think that it is a cleaner paradigm to encourage connection through the DSN rather than through parameters, as the DSN can contain all the information necessary for the connection, so I think it could make sense to have it be the default connection technique.

It would also drive parity for the implementation of other parameters for the parameter-driven instantiations of the method. This is to say that currently, my DSN looks like this: postgres://postgres:password@localhost:5433/database?sslmode=disable&connect_timeout=8

I'm able to pass these options through the DSN, whereas I don't think we've implemented the keyword arguments in order to allow some of those features through.

That said, given that you may prefer to give precedence to connect(Postgres, hostname) rather than connect(Postgres; hostname=hostname), I can instead just make connectdns(Postgres, dns).

iamed2 commented 9 years ago

I'd prefer connectdsn for now but as soon as I can work on this I'll be making everything keyword arguments at which point dsn will be given equal status :) At that point I'll also add in the extra options to the non-dsn connection method if libpq provides an easy way to do that.

cuevasclemente commented 9 years ago

Sounded good, but didn't work as well as I wanted:

`connect` has no method matching connect(::Function, ::Type{Postgres})
while loading In[21], in expression starting on line 1

 in get_postgres_table at In[20]:

I think the easiest thing for me to do is just keep that last implementation, which should make it easier for you to refactor this whenver you get around to it, but being aware that a do block will not work when using a DSN for now. I'm sure it would not be that much work to implement a solution, but it would probably require reworking a bit more of the library that you're already intending on working on. I'll push my changes; cheers!

iamed2 commented 9 years ago

Thanks! I plan on fixing up a few things as soon as I can and this will be included. I'm not entirely happy with any part of the library I wrote, to be honest, but I have plenty of ideas.