JuliaDatabases / PostgreSQL.jl

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

Adopt keyword arguments for connect function #42

Closed randomizedthinking closed 6 years ago

randomizedthinking commented 8 years ago

Better to use keyword parameters: requires updating DBI.connect. Leave it to future.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.6%) to 75.0% when pulling 36545180532d063ecbe3e0fabbb27aad39068dd0 on randomizedthinking:connparam into 0d13f023d359760b41653482f96e5303bdfc379a on JuliaDB:master.

codecov-io commented 8 years ago

Current coverage is 75.56%

Merging #42 into master will not change coverage

@@             master        #42   diff @@
==========================================
  Files             3          3          
  Lines           221        221          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            167        167          
  Misses           54         54          
  Partials          0          0          

Powered by Codecov. Last update 0d13f02...0d13f02

iamed2 commented 8 years ago

The environment variables are wrong and are unnecessary; libpq will pick them up if passed C_NULL for a parameter.

A null should also be represented with a Nullable instead of an empty string.

randomizedthinking commented 8 years ago

The signature of the connect function is now updated to accept NULL values. Can you review again?

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 75.566% when pulling 3fd5271b09bb24325dad9c67d1ed8d031ec814f6 on randomizedthinking:connparam into 0d13f023d359760b41653482f96e5303bdfc379a on JuliaDB:master.

iamed2 commented 8 years ago

The connect function should take Nullables, not C_NULL. Read http://docs.julialang.org/en/latest/manual/types/#nullable-types-representing-missing-values for more info.

randomizedthinking commented 8 years ago

I tried Nullable, yet the syntax is quite cumbersome. For instance, ASCIIString can't be assigned to Nullable{AbstractString} so we are forced to use Nullable("mydb") to call the connect function.

Likely, using keyword arguments would be a better approach.

iamed2 commented 8 years ago

If you'd like the keyword approach better, please make a PR to DBI to include that, and then implement it here.

You could still take the Nullable approach if you had a method that accepted Union{AbstractString, Nullable} and converted the inputs to Nullable{AbstractString}.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 75.566% when pulling 320b8887732317667798be596fa4faf855e43a7b on randomizedthinking:connparam into 0d13f023d359760b41653482f96e5303bdfc379a on JuliaDB:master.

randomizedthinking commented 8 years ago

The updated PR now takes keyword arguments for the connect function. Verified there is no need to modify the DBI module. The function connect(::Type{PostgreSQL.Postgres}; dsn::AbstractString) is converted to using positional argument connect(::Type{PostgreSQL.Postgres}, dsn::AbstractString). This is because keyword arguments are not part of the function signature; hence, we can only have one function with the given signature. Let me know how you think.