Bitmessage/PyBitmessage-2024-12-05
Archived
2
0
Fork 0
Code Issues 168 Pull Requests 53 Actions Packages Projects Releases 5 Wiki Activity

Refactor sqlthread and add create test cases #1999

Open
shekhar-cis wants to merge 1 commits from shekhar-cis/test_sqlthread_refactor into v0.6
pull from: shekhar-cis/test_sqlthread_refactor
merge into: Bitmessage:v0.6
Conversation 0 Commits 1 Files Changed 4 +558 -567
shekhar-cis commented 2022-07-13 10:14:15 +02:00 (Migrated from github.com)
Copy Link
No description provided.
PeterSurda (Migrated from github.com) requested changes 2022-07-15 03:27:45 +02:00
src/class_sqlThread.py
PeterSurda (Migrated from github.com) commented 2022-07-15 03:22:01 +02:00

the PR should use consistent language dialect. In some places in this PR verbs end with -se (UK) and other places with -ze (US). I don't think the project has a standard for this but I think it should be -ze.

the PR should use consistent language dialect. In some places in this PR verbs end with `-se` (UK) and other places with `-ze` (US). I don't think the project has a standard for this but I think it should be `-ze`.
src/class_sqlThread.py
@ -423,1 +147,4 @@
"""
Check if sqlite can store binary zeros.
"""
try:
PeterSurda (Migrated from github.com) commented 2022-07-15 03:27:13 +02:00

It may be helpful to have a common error handler or at least common error messages for errors. Or, perhaps a decorator would be helpful? It would wrap the whole method in a try/except, assuming we can refactor the rest of the code so that all the SQL operations can be done by dedicated methods.

It may be helpful to have a common error handler or at least common error messages for errors. Or, perhaps a decorator would be helpful? It would wrap the whole method in a `try`/`except`, assuming we can refactor the rest of the code so that all the SQL operations can be done by dedicated methods.
src/class_sqlThread.py
@ -517,3 +204,2 @@
self.conn.close()
logger.info('sqlThread exiting gracefully.')
self.initialize_sql("config_setting_ver_{}".format(settingsversion))
PeterSurda (Migrated from github.com) commented 2022-07-15 03:23:27 +02:00

this should be split into multiple methods

this should be split into multiple methods
src/class_sqlThread.py
@ -635,4 +227,4 @@
try:
self.conn.create_function("enaddr", 3, func=encodeAddress, deterministic=True)
except (TypeError, sqlite3.NotSupportedError) as err:
logger.debug(
PeterSurda (Migrated from github.com) commented 2022-07-15 03:24:41 +02:00

we should have a test of what happens if there is legacy string in the database when retrieving the value

we should have a test of what happens if there is legacy string in the database when retrieving the value
PeterSurda (Migrated from github.com) requested changes 2022-07-18 08:42:57 +02:00
src/class_sqlThread.py
PeterSurda (Migrated from github.com) commented 2022-07-18 08:42:03 +02:00

add comment # bump sql_schema_version by one

add comment `# bump sql_schema_version by one`
src/class_sqlThread.py
@ -593,1 +215,3 @@
os._exit(0)
logger.info('Created messages database file')
except Exception as err:
if str(err) == 'table inbox already exists':
PeterSurda (Migrated from github.com) commented 2022-07-18 08:40:22 +02:00

We should also handle FileNotFoundError, then log debug that the file is missing, and return.

We should also handle `FileNotFoundError`, then log debug that the file is missing, and return.
src/tests/test_sqlthread.py
PeterSurda (Migrated from github.com) commented 2022-07-18 08:32:54 +02:00

between 221 and 222 insert here a call to sql_schema_version

between 221 and 222 insert here a call to `sql_schema_version`
shekhar-cis (Migrated from github.com) reviewed 2022-07-18 14:37:41 +02:00
src/class_sqlThread.py
@ -593,1 +215,3 @@
os._exit(0)
logger.info('Created messages database file')
except Exception as err:
if str(err) == 'table inbox already exists':
shekhar-cis (Migrated from github.com) commented 2022-07-18 14:37:41 +02:00

I have checked that FileNotFoundError is for python3.
or we can do like

if six.PY2:
    FileNotFoundError = IOError

or

try:
    FileNotFoundError
except NameError:
    # py2
    FileNotFoundError = IOError

And IOError raises IOError(2, 'No such file or directory')

I have checked that `FileNotFoundError` is for `python3`. or we can do like ``` if six.PY2: FileNotFoundError = IOError ``` or ``` try: FileNotFoundError except NameError: # py2 FileNotFoundError = IOError ``` And `IOError` raises `IOError(2, 'No such file or directory')`
PeterSurda (Migrated from github.com) requested changes 2022-07-19 08:53:16 +02:00
src/class_sqlThread.py
PeterSurda (Migrated from github.com) commented 2022-07-19 08:48:45 +02:00 
except ValueError:
    return 0
``` except ValueError: return 0 ```
src/class_sqlThread.py
@ -576,0 +210,4 @@
try:
inbox_exists = list(self.cur.execute('PRAGMA table_info(inbox)'))
if not inbox_exists:
self.initialize_sql("initialize_schema")
PeterSurda (Migrated from github.com) commented 2022-07-19 08:51:21 +02:00

instead of custom_error maybe command

instead of `custom_error` maybe `command`
src/class_sqlThread.py
@ -593,1 +215,3 @@
os._exit(0)
logger.info('Created messages database file')
except Exception as err:
if str(err) == 'table inbox already exists':
PeterSurda (Migrated from github.com) commented 2022-07-19 08:45:13 +02:00

we seem to be missing a sqlite error handler. This should also log, but stronger severity (e.g. error), not debug

we seem to be missing a sqlite error handler. This should also log, but stronger severity (e.g. `error`), not `debug`
PeterSurda (Migrated from github.com) requested changes 2022-07-20 03:21:34 +02:00
src/class_sqlThread.py
PeterSurda (Migrated from github.com) commented 2022-07-20 03:15:54 +02:00

only return should be wrapped under try

only `return` should be wrapped under `try`
src/class_sqlThread.py
@ -594,0 +215,4 @@
logger.info('Created messages database file')
except Exception as err:
if str(err) == 'table inbox already exists':
logger.debug('Database file already exists.')
PeterSurda (Migrated from github.com) commented 2022-07-20 03:17:42 +02:00

these sections should also be in a separate function/method

these sections should also be in a separate function/method
PeterSurda (Migrated from github.com) commented 2022-07-20 03:18:44 +02:00

I don't know if there needs to be a variable `command, why not just use the string directly?

I don't know if there needs to be a variable `command, why not just use the string directly?
PeterSurda (Migrated from github.com) requested changes 2022-07-20 08:37:01 +02:00
src/class_sqlThread.py
PeterSurda (Migrated from github.com) commented 2022-07-20 08:35:14 +02:00

get rid of this return

get rid of this `return`
src/class_sqlThread.py
@ -422,1 +145,4 @@
def check_columns_can_store_binary_null(self):
"""
Check if sqlite can store binary zeros.
PeterSurda (Migrated from github.com) commented 2022-07-20 08:34:53 +02:00

return True

`return True`
src/class_sqlThread.py
@ -593,1 +215,3 @@
os._exit(0)
logger.info('Created messages database file')
except Exception as err:
if str(err) == 'table inbox already exists':
PeterSurda (Migrated from github.com) commented 2022-07-20 08:35:05 +02:00

return False

`return False`
PeterSurda (Migrated from github.com) requested changes 2022-07-21 03:04:24 +02:00
PeterSurda (Migrated from github.com) left a comment

The two changes I suggested should fix the tests too.

The two changes I suggested should fix the tests too.
src/class_sqlThread.py
PeterSurda (Migrated from github.com) commented 2022-07-21 03:00:19 +02:00

ok I didn't explain it well:

self.cur.execute(query, parameters)
try:
    return int(self.cur.fetchall()[0][0])
except ValueError, IndexError
    return 0

I added an IndexError just in case.

ok I didn't explain it well: ``` self.cur.execute(query, parameters) try: return int(self.cur.fetchall()[0][0]) except ValueError, IndexError return 0 ``` I added an IndexError just in case.
src/class_sqlThread.py
@ -593,1 +215,3 @@
os._exit(0)
logger.info('Created messages database file')
except Exception as err:
if str(err) == 'table inbox already exists':
PeterSurda (Migrated from github.com) commented 2022-07-21 03:01:12 +02:00

return False should be after the try/except block so that it's consistent (returns True if no exception, otherwise returns False)

`return False` should be after the `try/except` block so that it's consistent (returns `True` if no exception, otherwise returns `False`)
PeterSurda (Migrated from github.com) requested changes 2022-07-21 11:57:47 +02:00
src/class_sqlThread.py
@ -473,168 +191,247 @@ class sqlThread(threading.Thread):
try:
self.cur.execute(''' VACUUM ''')
except Exception as err:
PeterSurda (Migrated from github.com) commented 2022-07-21 11:57:28 +02:00

replace with error_handler

replace with `error_handler`
shekhar-cis (Migrated from github.com) reviewed 2022-07-21 13:12:50 +02:00
src/class_sqlThread.py
@ -473,168 +191,247 @@ class sqlThread(threading.Thread):
try:
self.cur.execute(''' VACUUM ''')
except Exception as err:
shekhar-cis (Migrated from github.com) commented 2022-07-21 13:12:50 +02:00

error_handler is in else block.

`error_handler` is in `else` block.
PeterSurda (Migrated from github.com) reviewed 2022-07-21 13:21:03 +02:00
src/class_sqlThread.py
@ -473,168 +191,247 @@ class sqlThread(threading.Thread):
try:
self.cur.execute(''' VACUUM ''')
except Exception as err:
PeterSurda (Migrated from github.com) commented 2022-07-21 13:21:03 +02:00

the error handling should be unified, this here isn't a special case

the error handling should be unified, this here isn't a special case
shekhar-cis (Migrated from github.com) reviewed 2022-07-21 14:06:20 +02:00
src/class_sqlThread.py
@ -473,168 +191,247 @@ class sqlThread(threading.Thread):
try:
self.cur.execute(''' VACUUM ''')
except Exception as err:
shekhar-cis (Migrated from github.com) commented 2022-07-21 14:06:20 +02:00

Added inside error_handler.

Added inside `error_handler`.
PeterSurda (Migrated from github.com) requested changes 2022-07-22 03:20:41 +02:00
src/class_sqlThread.py
PeterSurda (Migrated from github.com) commented 2022-07-22 03:19:46 +02:00

this should also use error_handler

this should also use `error_handler`
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
PeterSurda
Labels
Clear labels   
bug

Something isn't working as it's supposed to

  
build

Related to building or build system

  
dependencies

Pull requests that update a dependency file

  
developers

The issue is relevant mainly for developers rather than users

  
documentation

   
duplicate

   
enhancement

New feature

  
formatting

Code or UI text formatting

  
invalid

   
legal

   
mobile

Frontend for mobile devices (kivy)

  
obsolete

   
packaging

   
performance

   
protocol

Protocol change

  
question

   
refactoring

   
regression

Some issue, fixed in the past, appears again

  
security

   
test

   
translation

   
usability

   
wontfix
No Label
bug
build
dependencies
developers
documentation
duplicate
enhancement
formatting
invalid
legal
mobile
obsolete
packaging
performance
protocol
question
refactoring
regression
security
test
translation
usability
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Bitmessage/PyBitmessage-2024-12-05#1999
No description provided.
Delete Branch "shekhar-cis/test_sqlthread_refactor"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?