Retrieve all table names from existing SQLite file. by transfluxus · Pull Request #72 · piskvorky/sqlitedict

transfluxus · 2018-01-18T11:11:28Z

Add get_tablenames utility method that retrieves all table names from the file with SQLite database.

FIxes #71.

piskvorky · 2018-01-18T16:44:01Z

+def print_tablenames(filename):
+    """print tablenames and return them as list. Returns empty list if file does not exist (instead of creating that file)"""
+    if not os.path.isfile(filename):
+        logger.warning('file %s does not exist' %filename)


Explicit fail (exception) better?

Also, spaces around %.

piskvorky · 2018-01-18T19:22:38Z

    if not os.path.isfile(filename):
-        logger.warning('file %s does not exist' %filename)
-        return
+        raise IOError('file %s does not exist' % (filename))


What does sqlite3 do when asked to connect to a file that doesn't exist? Isn't that "default" error better?

it's weird. It creates a database of that name and returns an empty list

I see. That's really bad behaviour from sqlite3 -- I agree raising an exception is much better.

ownport · 2018-01-18T20:49:35Z

As proposal, instead of printing tables maybe it will be better to return the list of them and user will decide how to use this list?

menshikh-iv

Good start, don't forget about tests (need to add tests for new functionality)

menshikh-iv · 2018-01-19T06:25:14Z

+    conn = sqlite3.connect(filename)
+    res = conn.execute("SELECT name FROM sqlite_master WHERE type='table';")
+    tablenames = [name[0] for name in res]
+    print(tablenames)


return tablenames enough (no need to print it)

menshikh-iv · 2018-01-19T06:26:52Z

    return loads(bytes(obj))


+def print_tablenames(filename):


IMO better to move it to SqliteDict class and use as method, wdyt @transfluxus ?

possible. but that would create already at least one table (unnamed). could be a static method. The way use it is to cal it before creating the SqliteDict obj.

But what for? I don't see a problem here.

transfluxus · 2018-01-19T18:35:22Z

instead of this:

db = SqliteDict('testDB.sqlite')
db = SqliteDict('testDB.sqlite',tablename='vocab')
db.get_tablenames()

I would rather have something like this:

dbs = {tablename : SqliteDict('testDB.sqlite',tablename) for tablename in get_tablenames('testDB.sqlite')}

transfluxus · 2018-01-19T18:36:35Z

but just let me know, what you prefer. it includes both versions now

piskvorky · 2018-01-20T13:55:28Z


+    def get_tablenames(self):
+        """print tablenames and return them as list"""
+        res = self.conn.select("SELECT name FROM sqlite_master WHERE type='table';")


-1 on code duplication, it makes maintenance a headache. Keep it DRY.

Can you refactor the core functionality to live in only one place? Possibly being used/called from multiple places, that's fine.

transfluxus · 2018-01-20T19:57:44Z

Of course. That's what I came up with in the end, since the db connection/cursor are in the run of the multithread obj.

I'm not used to write test :), but I added some. Hope they make sense

menshikh-iv · 2018-01-22T06:31:56Z

@@ -0,0 +1,6 @@
+{


unrelated file, please remove

aii. sneaked in...

menshikh-iv · 2018-01-22T06:33:23Z

+        self.assertEqual(db1.get_tablenames(), ['table1'])
+        db2 = sqlitedict.SqliteDict(fname,tablename='table2') 
+        self.assertEqual(db1.get_tablenames(), ['table1','table2'])
+        db1.close()


with SliteDict(...): self.assertEqual

better than db = ; db.close()

will do that

is there a reason you're not having
from sqlitedict import SliteDict ?

ok, I did, makes things look cleaner.
having with makes things look better indeed. However my test is now the only one doing that. looks a bit inconsistent...

@transfluxus I think we'll cleanup codebase later, don't worry :)

menshikh-iv · 2018-01-22T06:33:45Z

+
+    def test_tablenames(self):
+        fname = norm_file('tests/db/tablenames-test-1.sqlite')
+        db = sqlitedict.SqliteDict(fname) 


what's about an empty file?

not sure what you mean. in any case table unnamed will be created

@transfluxus aha, OK!

menshikh-iv · 2018-01-22T06:35:09Z

    return loads(bytes(obj))


+def get_tablenames(filename):


probably better to remove this function & move all code to method, wdyt?

you mean just a class method?
I can do that. just makes use case look unpractical. see above

@transfluxus what's about static method?

- remove temp ipynb

adjust tests

menshikh-iv · 2018-01-24T07:15:16Z

            yield rec

-    def select_one(self, req, arg=None):
+    def select_one(self, req, arg=None):    


leading space (PEP8)

menshikh-iv · 2018-01-24T07:16:17Z

 from sys import version_info
 major_version = version_info[0]
-
+from accessories import norm_file


unused import

menshikh-iv · 2018-01-24T07:17:03Z

+        """get the names of the tables in an sqlite db as a list"""
+        if not os.path.isfile(filename):
+            raise IOError('file %s does not exist' % (filename))   
+        conn = sqlite3.connect(filename)


what's about with here too?

change test imports

menshikh-iv · 2018-01-26T08:10:26Z

Thank you @transfluxus! Congratz with first contribution 🥇

Ramin and others added 2 commits January 18, 2018 12:08

add print_tablenames method

387bb21

Update sqlitedict.py

5096264

piskvorky requested changes Jan 18, 2018

View reviewed changes

Ramin and others added 3 commits January 18, 2018 19:40

raise exception when file does not exist

bed9b09

raise exception when file does not exist

d31f7e0

typo

4963a3e

piskvorky reviewed Jan 18, 2018

View reviewed changes

menshikh-iv suggested changes Jan 19, 2018

View reviewed changes

Ramin added 2 commits January 19, 2018 19:30

added method to sqliteDict

99b7e5d

added method to sqliteDict

87e6595

remove test-ipynb

2c901ce

piskvorky requested changes Jan 20, 2018

View reviewed changes

Ramin added 2 commits January 20, 2018 20:46

dried it. added tests

ed99659

fixed tests

0ff35bc

cut one line

726c7ea

menshikh-iv suggested changes Jan 22, 2018

View reviewed changes

Ramin added 2 commits January 22, 2018 19:50

- change test structure

c077f6c

- remove temp ipynb

keep only one method and make it static

4cb5720

adjust tests

menshikh-iv suggested changes Jan 24, 2018

View reviewed changes

use with in get_tablenames method

3ded274

change test imports

menshikh-iv changed the title ~~add print_tablenames method~~ Jan 26, 2018

menshikh-iv merged commit 289c022 into piskvorky:master Jan 26, 2018

piskvorky changed the title ~~Add get_tablenames method. Fix #71~~ Sep 18, 2018

Conversation

transfluxus commented Jan 18, 2018 • edited by piskvorky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ownport commented Jan 18, 2018

menshikh-iv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

transfluxus commented Jan 19, 2018

transfluxus commented Jan 19, 2018

Choose a reason for hiding this comment

transfluxus commented Jan 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

transfluxus Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menshikh-iv commented Jan 26, 2018

Labels

4 participants

transfluxus commented Jan 18, 2018 •

edited by piskvorky

Loading

transfluxus Jan 22, 2018 •

edited

Loading