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

Commented out default driver name for turbodbc #57

Closed
wants to merge 1 commit into from

Conversation

pmetzner
Copy link

The default driver name kept sabotaging queries, so I hotfixed it by commenting it out.

@pmetzner pmetzner mentioned this pull request Mar 16, 2017
@jank
Copy link

jank commented Apr 18, 2017

@MathMagique can you have a look at this PR? Also have a look at PR #59 where the default driver name was wrong for Windows (and fixed) as I am not sure if this is related.

@MathMagique
Copy link
Contributor

MathMagique commented Apr 18, 2017

@BY-jk Yes, it seems that would be related. It is not a turbodbc-specific issue (PR #59 is about pyodbc). The issue is that sqlalchemy_exasol assumes a default driver name that is neither universal nor necessary. Changing the default as in #59 will break code for everybody that relied (implicitly) on the previous default.

@pmetzner's pull request shifts the responsibility to set the driver to the user, and that is the only person who can really know what the driver's name is. Also, using the data source name in the connection options usually implies that the driver is already configured correctly, so there is no need to mention it again. That seems to be what Paul ran into.

My advice would be to extend this pull request to the pyodbc part as well, and to remove all traces that there ever was a default. The documentation would then need to be extended with a note saying that one needs to specify the driver manually, and how to find out the driver's name. This would be the honest option and should avoid further pull requests of users tring to change the default to what suits their system.

@jank jank mentioned this pull request Jul 21, 2017
@jank
Copy link

jank commented Jul 21, 2017

Closed in favor of: #66

@jank jank closed this Jul 21, 2017
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.

None yet

3 participants