Fix typo made in #1964

[vnlitvinov]

#1964 changed the API for OmniSciDBClient but didn't change all usages of device= parameter when constructing a client. In my case a connection.database() call was failing.

[xmnlab]

@vnlitvinov thanks for caching that up.

could you add tests for that to omniscidb/tests/?

would be good to have a test for the 2 cases, when the database name is the same of the current_database and for a different name (maybe you can use the omnisci database or you will need to create a temporary database for the test.

thanks!

[vnlitvinov]

@abykovsk could you please take a stab at making required tests?

[datapythonista]

@abykovsk could you please take a stab at making required tests?

There should be a test that failed before merging #1964, so this never broke. There is this tests but since name == None the breaking code is never called. I guess you can add a pytest parametrization to that test, so it's called with name == None but also to a different database name to the current want.

[pep8speaks]

Hello @vnlitvinov! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-01 15:29:44 UTC

[vnlitvinov]

would be good to have a test for the 2 cases, when the database name is the same of the current_database and for a different name

I have added a separate test (the case where database name is same as current behaves exactly the same as with database name being None and is already handled by test_database_layer).
I tried parametrizing it first, but felt it would end up too complex (and it would actually test two unrelated things which isn't good for a unit test), so I wrote a new test instead.

[xmnlab]

LGTM, thanks @vnlitvinov for working on that!

[vnlitvinov]

Anything left for me to do to make this accepted?

[jreback]

looks ok, but tests are failing

[vnlitvinov]

I know, we're looking at it. The test should probably be enabled only for SQL-like backends.

[vnlitvinov]

@jreback ping :)

[jreback]

@vnlitvinov omnisci is currently not being tested, so all merging on hold

[vnlitvinov]

What's the reason for this? Can we help with that being fixed?

[jreback]

#2201

[datapythonista]

@vnlitvinov can you merge master, and check if the CI is still green please? CI is working for omnisci, we can move forward with this now

[vnlitvinov]

@datapythonista I've rebased instead for a cleaner history.

[vnlitvinov]

The CI failures look strange and unrelated to our changes... can I do something to get this merged?

[jreback]

@vnlitvinov we need a test that fails on master and is fixed by this change

[vnlitvinov]

@jreback it would be nice to see this requirement a bit earlier than in almost 3 months... in a PR fixing a typo (original diff was one line).

Am I reading this right that you want me to make a separate PR (with a failing test) which would turn master CI red, and only then fix that test? All for 1 line (and two words) worth of a change?..

Also note there is a test added that would fail on master if ran now: https://github.com/ibis-project/ibis/pull/2106/files#diff-2268971ed7d3b780e597e36b7a466045R203

[jreback]

i commented quite a while ago on this

one line or a million it doesn’t matter

if something is claimed to be fixed then it needs a test

often the test are way longer than the patch

i want a test included with this PR
one that fails in master

[vnlitvinov]

i want a test included with this PR
one that fails in master

https://github.com/ibis-project/ibis/pull/2106/files#diff-2268971ed7d3b780e597e36b7a466045R203

[jreback]

changes look ok. rebase on master and some comments, ping on green.

[vnlitvinov]

@jreback I think we addressed all your comments (so I marked them as resolved), and after merge with master CI is finally green.

[jreback]

thanks @vnlitvinov