-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix "raise_if_test" behaviour #241
Conversation
MorrisNein
commented
Nov 25, 2023
•
edited
Loading
edited
- Create a method "log_or_raise" to replace the argument "raise if test"
- Write a test for this method
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
==========================================
- Coverage 71.96% 71.94% -0.03%
==========================================
Files 136 136
Lines 8130 8127 -3
==========================================
- Hits 5851 5847 -4
- Misses 2279 2280 +1 ☔ View full report in Codecov by Sentry. |
golem/core/log.py
Outdated
@@ -208,8 +208,8 @@ def __repr__(self): | |||
return self.__str__() | |||
|
|||
|
|||
def raise_if_test(msg, **kwargs): | |||
if kwargs.get('raise_if_test', False) is True and is_test_session: | |||
def raise_if_test(msg: object, __raise_if_test: bool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А зачем аргумент функции делать __?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вообще для того, имя переменной не совпадало с именем функции.
"__
" делает т.н. "name mangling", т.е. изменяет имя аргумента на "LoggerAdapter__raise_if_test
". Но, думаю, в этой ситуации можно было обойтись и просто другим именем "raise_if_test_
"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тоже не актуально
golem/core/log.py
Outdated
@@ -161,42 +161,42 @@ def process(self, msg, kwargs): | |||
return '%s - %s' % (self.extra['prefix'], msg), kwargs | |||
|
|||
def debug(self, msg, *args, **kwargs): | |||
raise_if_test(msg, **kwargs) | |||
raise_if_test(msg, kwargs.pop('raise_if_test', False)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А почему понадобилось передавать это в kwargs, причем для конкретного вызова логгера?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Для совместимости с интерфейсом класса-родителя (logging.LoggerAdapter).
Если мы захотим добавить такой аргумент, то нам придётся явно полностью повторить интерфейс, и уже после всех именованных аргументов указать raise_if_test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Видимо в обновленной версии уже не актуально
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Верно
golem/core/log.py
Outdated
@@ -156,50 +158,43 @@ def __init__(self, logger: logging.Logger, extra: dict): | |||
self.logging_level = logger.level | |||
self.setLevel(self.logging_level) | |||
|
|||
def log_or_raise( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cтоит добавить докстринг с пояснением всех этих сложных параметров.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Добавил
Hello @MorrisNein! 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 2023-11-27 12:49:31 UTC |