Codebase refresh #6
Conversation
…int, replace for concurrent-log-handler
| from os import path | ||
| from pprint import pprint | ||
| from sys import exc_info | ||
|
|
||
| from bismuthcore.helpers import BismuthBase | ||
| from bismuthcore.bismuthcore.helpers import BismuthBase |
There was a problem hiding this comment.
The top level bismuthcore dir of the repo is an extra dir, not part of the library.
So there is no bismuthcore.bismuthcore in the module and this fix, as other similar imports in other files, adding an extraneous bismuthcore, would break anything requiring bismuthcore.
The pypi config therefore may be wrong as well.
If an IDE suggested that, then its config or lib paths are wrong.
There was a problem hiding this comment.
Something is odd with the setup.py, the only suggestion I received from my IDE was that imports could not resolve. Pytest confirmed that fact. I'll address in setup.py.
| @@ -87,11 +87,11 @@ def create(db, sql: tuple): | |||
|
|
|||
| try: | |||
| remove('ledger_new.db') | |||
| except: | |||
| except Exception: | |||
There was a problem hiding this comment.
I do agree a bare "except" is not PEP, but be aware "except Exception" does not catch the same things as except alone. "except BaseException" would catch the same events.
Deciding which except should catch a KeyboardInterrupt or not may be non trivial.
In doubt, I would not change the current working, therefore use BaseException if really the bare except is too hard for some eyes.
A proper exception handling should anyway detail the possible exceptions more in depth, and would bloat the codebase quite a bit so I would not advise it.
There was a problem hiding this comment.
Perhaps throwing in BaseException for catching system-exiting exceptions, above the except Exception should address this concern without bloating the codebase too much.
| except: | ||
| pass | ||
| except Exception as e: | ||
| print("Unknown error occurred during wallet preview. Error: {}".format(e)) |
There was a problem hiding this comment.
What is the rationale for prints instead of the self.app_log used everywhere?
There was a problem hiding this comment.
self.app_log is not an available attribute. Also, I intended to ignore legacy/, but failed to ignore it when I did a search for except: across the codebase. I may look into removing these commits in legacy/ in order to avoid confusion.
| except: | ||
| pass | ||
| except Exception as e: | ||
| self.app_log.debug(f"Connection to {pair[0]} {pair[1]} failed: {e}") |
There was a problem hiding this comment.
The "pair" info is already logged with info level just above.
So in case of an exception, the short "not connectable" (typo in codebase) message is supposed to refer to the "connection" line just above.
I'm not sure there's a need to duplicate the info.
There was a problem hiding this comment.
You're are correct. Will look into removing changes in legacy/
| @@ -451,9 +453,9 @@ def consensus_remove(self, peer_ip): | |||
| self.app_log.info(f"Consensus opinion list: {self.peer_opinion_dict}") | |||
| self.app_log.info(f"Will remove {peer_ip} from consensus pool {self.peer_opinion_dict}") | |||
| self.peer_opinion_dict.pop(peer_ip) | |||
| except: | |||
| except Exception as e: | |||
| self.debug(f"Error removing {peer_ip} from consensus pool: {e}") | |||
There was a problem hiding this comment.
dup log (plus typo likely, self.debug instead of self.app_log.debug)
| tries, timeout = self.tried[host_port] # noqa: F841 | ||
| except Exception as e: | ||
| # unknown host for now, never tried. | ||
| self.app_log.debug(f"Error getting tries for {host_port}: {e}") |
There was a problem hiding this comment.
This is not an error, this is the normal working for an host we don't know yet.
This is ok and should not generate a log.
The pythonic way would be a use a defaultDict with (0,0) or .get(host_port, (0,0)) and get rid of the try /except alltogether.
| @@ -54,7 +54,7 @@ def _send(self, data, slen=SLEN, retry=True): | |||
| self.sdef.settimeout(LTIMEOUT) | |||
| # Make sure the packet is sent in one call | |||
| sdata = str(json.dumps(data)) | |||
| res = self.sdef.sendall(str(len(sdata)).encode("utf-8").zfill(slen)+sdata.encode("utf-8")) | |||
| _ = self.sdef.sendall(str(len(sdata)).encode("utf-8").zfill(slen)+sdata.encode("utf-8")) | |||
There was a problem hiding this comment.
I would even remove the _ = for better readability, or check res for added safety, make sure all was indeed sent.
There was a problem hiding this comment.
Definitely a check would be optimal. A few improvements could be made to this line altogether.
Resubmitting PR without formatting/linting changes. Only functional changes are present in this request. Please pull, test, run checks, and review before merging.