Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mssql multi driver support #10

Closed
wants to merge 9 commits into from

Conversation

drmuey
Copy link

@drmuey drmuey commented Jan 29, 2016

Review

Changes:

Testing:

  • prove -lw t == All tests successful.

@theory
Copy link
Collaborator

theory commented Feb 1, 2016

This does way more than I had expected. I've revised it and committed the changes in 05061bf..2a09bcc. I have it all in the ado branch because, frankly, I find ADO pretty impenetrable. From some Googling, it looks like the parameters change based on the Provider parameter. So I think we really need someone who knows ADO well to vet this.

@theory theory closed this Feb 1, 2016
@drmuey
Copy link
Author

drmuey commented Feb 1, 2016

yeah it does more than single change to mssql’s dbi_dsn because if you don't rebless the internal object you get, say ODBC’s attributes, instead of Sybase’s attributes when when you:

$u->dbi_dsn('Sybase');

You can see that in action if you change the bless() calls to the $class->new(…)->dbi_dsn() version and then run the dbi.t tests.

With the rebless you must have the special dbi_dsn() in each related driver so that we can switch between them ok when we $u->dbi_dsn('Sybase') then $u->dbi_dsn('ADO'); then $u->dbi_dsn('ODBC');.

The other advantage of reblessing is you change it once and it sticks. Otherwise in any code base that calls dbi_dsn w/ an alternate driver they have to include the driver argument in every call to dbi_dsn()

Instead of, say, an initialization step that calls dbi_dsn($driver) to set the driver and then everywhere else just driver agnostically uses dbi_dsn() since it can assume its already set correctly.

All of that is why I included so many tests proving the behavior (dbi.t) and permutations (mssql.t) of changing one sticking.

HTH

@theory
Copy link
Collaborator

theory commented Feb 1, 2016

You don't want to rebless the object, because that changes out from under who is using it. I have the code instead bless a copy. That avoids the need to have the argument-handling dbi_dsn in the other classes, because the object's class does not change. Changing the object out from under the user is a very bad idea, though I do get the appeal of the change "sticking".

@drmuey
Copy link
Author

drmuey commented Feb 1, 2016

ok, but its not just about it sticking, since its essentially just calling another class’s dbi_dsn it will be wrong sometime because it will be using the original class for other things, e.g. attributes.

e.g. if you dbi_dsn('Sybase') it will use ODBC attributes like Server instead of Sybase's host.

that is why the entire underlying object had to change.

HTH!

@theory
Copy link
Collaborator

theory commented Feb 1, 2016

Well, the URIs are for a database engine, not for the DBI. This way, dbi_dsn does the right thing, and there is no (apparent) magical changing of the class without the user noticing.

I could see an argument for casting a URI from one class to another. You can do that by changing the engine:

perl -MURI -E 'my $uri = URI->new("db:mssql:"); $uri->engine("sybase"); say $uri'    
db:sybase:

But I don't think it makes sense to do in dbi_dsn.

@drmuey
Copy link
Author

drmuey commented Feb 1, 2016

ok, let me know if I can be of any assistance, thanks :)

@drmuey
Copy link
Author

drmuey commented Feb 3, 2016

@theory If ADO is weird, what do you think about calling ADO support experimental w/ a pointer to a github issue asking for some assistance w/ it? That way we can do mssql support and not worry too much about ADO being a problem. Just a thought :)

@theory
Copy link
Collaborator

theory commented Feb 3, 2016

Good idea. Added #11. I'll push out a release pointing to it today.

@theory
Copy link
Collaborator

theory commented Feb 3, 2016

Okay, v0.17 is on its way to CPAN.

@drmuey
Copy link
Author

drmuey commented Feb 3, 2016

you are über fast ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants