Audit after join, Audit Queueing, Swagger UI #38
Conversation
Xyaren
commented
Sep 22, 2020
•
Xyaren
commented
- Add Swagger UI on / route
- Add Audit on join (prioritized)
- Enable queued audit processing
- Move audit functions to separate service
- allow cli arguments to be passed via ENV variables
- allow log level and filepath to be provided via args
- fixed some linter warnings
- fix bug that caused leftover guild icons on the server on deleting the guild
- slightly improved performance of audit for users that are in the Database but not in the Ts DB (flipped if-conditions)
- remove reinit of ts connection as this is working with listeners and "state" of the connection. (Name, Channel etc)
Xyaren
added
enhancement
* Add Audit on join (prioritized) * Enable queued audit processing * Move audit functions to separate service * allow arguments to be passed via ENV variables * allow log level and filepath to be provided via args * fixe some linter warnings * fix bug that caused leftover guild icons on the server on deleting the guild
| /health: | ||
| get: | ||
| summary: Healthckeck | ||
| operationId: helathCheck |
There was a problem hiding this comment.
There seems to be a typo here.
There was a problem hiding this comment.
Still seeing it as "helathCheck" on my end.
# Conflicts: # bot/guild_service.py
* fix some ts3 connection usages * fix exception handling for timeouts
* refactor permission requests (TsFacade) to use a single request for all permissions instead of one per permission
# Conflicts: # bot/TS3Bot.py # bot/ts/ThreadSafeTSConnection.py
…dit-queue # Conflicts: # .github/workflows/test.yml
|
@ogrady As there have been some dramatic changes, I would like to request a re-review :) |
| /health: | ||
| get: | ||
| summary: Healthckeck | ||
| operationId: helathCheck |
There was a problem hiding this comment.
Still seeing it as "helathCheck" on my end.
| @@ -20,11 +21,21 @@ def __init__(self, ts3_connection: ThreadSafeTSConnection): | |||
| def close(self): | |||
| self._ts3_connection.close() | |||
|
|
|||
| def __str__(self): | |||
| return f"TS3Facade[{self._ts3_connection}]" | |||
There was a problem hiding this comment.
I think we should settle on a consistent style for formatting strings. %-style seems to be the more prevalent one in this project rn, would you be fine with that?
There was a problem hiding this comment.
Log statement are using %s, for performance reasons, as the String is not built when not logged. (https://docs.python.org/3/howto/logging.html#optimization)
This on the other hand will always be "formatted" when called, always.
f Style strings are much more readable in my opinion and should be favored if possible.
Not only are they more readable, more concise, and less prone to error than other ways of formatting, but they are also faster!
Great readup: https://realpython.com/python-f-strings/
bot/ts/TS3Facade.py
Outdated
| try: | ||
| resp = self._ts3_connection.ts3exec_raise(lambda tc: tc.wait_for_event(timeout=timeout)) | ||
| except TS3TimeoutError: | ||
| return None |
There was a problem hiding this comment.
resp = NoneSame effect but structured.
There was a problem hiding this comment.
I'm much more of a fan of early returns, especially in a indentation based language like python.
It makes diffs much more readable for example if code gets added or removed.
There was a problem hiding this comment.
This is arguably an early return in this particular case, as the whole function is just one statement (other than that, not really in line with SE-discussion), but same reasoning as before: putting cleanup code below except is then skipped altogether when catching an exception.
There was a problem hiding this comment.
Cleanup code should be handled in a "finally" block or even better using a "with" statement.
Yes this is only one small function, but in terms of extendability the same rule applies here.
| @@ -19,6 +19,11 @@ def default_exception_handler(ex): | |||
| return ex | |||
|
|
|||
|
|
|||
| def raise_exception_handler(ex): | |||
There was a problem hiding this comment.
Not really a fan. This is what signal_exception_handler is for, adhering to the functional style of this part of the code, losely based on https://hackage.haskell.org/package/base-4.14.0.0/docs/Data-Either.html
There was a problem hiding this comment.
This would require manually raising the error through the whole callstack during the event loop. This leads to more clutter than it is already.
This is basically what Exceptions are made for.
Using exceptions Errors are not silently ignored if not looked for explicitly. You have to deal with the error explicitly sooner or later. This inevitably leads to more reliant code.
There was a problem hiding this comment.
Absolutely! And I am not particularly a fan of raising exceptions through multiple layers of the call stack (read: goto), unless they signal absolutely application-breaking circumstances in which any cleanup can be bypassed (this probably ultimately lead to the invention of the finally block, which basically is spaghetti to me).
There was a problem hiding this comment.
That is the case here. We are talking about Connection Problems, which can not be dealt with where they occur.
|
Update: |
|
Pylint is currently restricted to Python 3.8.x (pylint-dev/pylint#3882 (comment)) |
