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

Second version of connection_parameters patch. #99

Closed
wants to merge 6 commits into from

Conversation

vadz
Copy link
Member

@vadz vadz commented Mar 9, 2013

This one should take into account all the comments on the previous one. In particular, I renamed the new option to the lower case.

vadz added 6 commits March 9, 2013 13:11
Right now this is just a trivial wrapper for the backend and connection string
and no extra functionality but it will be extended with connection options
support in the subsequent commits.

No changes in behaviour, this is a pure refactoring.

Signed-off-by: Vadim Zeitlin <vz-soci@zeitlins.org>
No real changes, but just use "class" for consistency.

Signed-off-by: Vadim Zeitlin <vz-soci@zeitlins.org>
…string.

There are still no real changes as only the connect string part of the
connection parameters is currently used, but this will make it possible to use
other information stored in connection_parameters from the backend code in the
future.

Also replace session::lastFactory_ and lastConnectString_ with a single
lastConnectParameters_ making the code slightly shorter and simpler.
It should be to connections.html, not the (non existent) basics.html.
Allow passing arbitrary options via connection_parameters and add support for
an option allowing to specify the "driver completion" mode, i.e. the degree of
interactivity of the driver, in the ODBC backend.

By default now do ask the user if the DSN doesn't include the user name and/or
password for the connection but the old behaviour is still available if
ODBC_OPTION_DRIVER_COMPLETE option is set to SQL_DRIVER_NOPROMPT before
opening the session.
Document the new session ctor taking connection_parameters and the class
itself in the references page.
@mloskot
Copy link
Contributor

mloskot commented Mar 9, 2013

I wonder, simple update to commits of previous pull request #98 wouldn't work?
I'm still learning GitHub thus I'm curious, because Using Pull Requests doc claims that additional commits are fully supported.

It looks some network problems on travis broke the build, not sure if it will refresh, but let's ignore and build locally.

@mloskot
Copy link
Contributor

mloskot commented Mar 9, 2013

I also tested this one with VS2012 and ODBC (MySQL, PostgreSQL, SQL Server). All works.

@mloskot
Copy link
Contributor

mloskot commented Mar 9, 2013

@vadz Could you make some "fake" commit in your params2 branch, for example add/remove comment or line, and push it to your remote params2 branch. I wonder if it would trigger travis rebuild.

@vadz
Copy link
Member Author

vadz commented Mar 9, 2013

I rewrote some commits instead of just adding new ones, which is why I've created a new branch and not just updated the old one. But the code is functionally identical with the old version (the only changes are renaming and the order of initialization changes), so I think it should be safe to commit it even without Travis check.

@mloskot
Copy link
Contributor

mloskot commented Mar 9, 2013

@vadz Understood. Yes, I agree it's safe to commot, I was just wondering if we should give @pfedor, @vnaydionov chance to look at it too, but we've been discussing it on the list already and no objections were raised, so go ahead and merge your work.

Regarding Travis, it seems the setup is broken and I'm looking at it now.

@mloskot
Copy link
Contributor

mloskot commented Mar 9, 2013

Update on Travis, apparently they just have switched to 64-bit so installing Oracle libraries from oss.oracle.com repository is failing.

I'll have to work on it

@mloskot
Copy link
Contributor

mloskot commented Mar 9, 2013

I had to disable Oracle on Travis, it needs more work than I thought to support it for 64-bit build.

Meanwhile, build 223 reports ODBC tests on 64-builds are failing.

@mloskot
Copy link
Contributor

mloskot commented Mar 10, 2013

I just tested 64-bit build on Windows using VS2012, ODBC backend with 64-bit ODBC drivers for MySQL and SQLite3 too, all works and test2() passes. I still haven't checked what's going wrong with ODBC 64-bit build on Linux at travis-ci.org. I may have to check tomorrow.

Meanwhile, I've added some verbose output to test2() (SHA: 2efba38):

1: Test command: /home/travis/build/SOCI/soci/src/_build/bin/soci_odbc_test_mysql "FILEDSN=/home/travis/build/SOCI/soci/src/_build/../backends/odbc/test/test-mysql.dsn"
1: Test timeout computed to be: 9.99988e+06
1: 
1: SOCI ODBC with MySQL Tests:
1: 
1: 
1: SOCI Common Tests:
1: 
1: test 0 passed
1: test 1 passed
1: test 2 passed
1: 0 -> 0
1: 1 -> 1
1: 2 -> 2
1: 3 -> 3
1: 4 -> 0
1: soci_odbc_test_mysql: /home/travis/build/SOCI/soci/src/core/test/common-tests.h:804: void soci::tests::common_tests::test3(): Assertion `i2 == vec[i]' failed.
1/7 Test #1: soci_odbc_test_mysql .............***Exception: Other  0.07 sec

but it should list until 99 -> 99.

@vadz
Copy link
Member Author

vadz commented Mar 13, 2013

In absence of comments from @pfedo and @vnaydionov I'm going to merge this in as I'd like it to be part of 3.2.0 and also because these changes touch a lot of files and the risk of conflicts is relatively important -- I've already had to fix another one in DB2 code -- and I don't believe this patch is responsible for ODBC tests failures in 64 bits (this is obviously something that needs to be fixed but not related to these changes).

@vadz vadz closed this Mar 13, 2013
vadz added a commit that referenced this pull request Mar 13, 2013
@mloskot
Copy link
Contributor

mloskot commented Mar 13, 2013

@vadz Great, thanks! Yes, the ODBC failures we're observing are not related to your submission.

A general note for future:
IMHO, lack of comments is either lack of objection or lack of review. In the latter case, we can't afford deferring merges for too long due to increasing risk of conflicts (let's not waste time on boring maintenance). So, let's simply assume that no objections or no comments indicate acceptance (especially if we have discussed a topic on the mailing list), plus tests have been successfully run. Then, pull request has green light :)

@ghost ghost assigned vadz Jun 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants