Closed knoepfel closed 2 years ago
Merging #515 (2fbf2af) into master (d7f4401) will decrease coverage by
0.04%
. The diff coverage is100.00%
.:exclamation: Current head 2fbf2af differs from pull request most recent head 39f7430. Consider uploading reports for the commit 39f7430 to get more accurate results
@@ Coverage Diff @@
## master #515 +/- ##
==========================================
- Coverage 94.12% 94.07% -0.05%
==========================================
Files 44 44
Lines 2742 2736 -6
Branches 392 392
==========================================
- Hits 2581 2574 -7
Misses 122 122
- Partials 39 40 +1
Flag | Coverage Δ | |
---|---|---|
python-3.6 | 93.50% <100.00%> (-0.02%) |
:arrow_down: |
python-3.9 | 93.60% <100.00%> (-0.06%) |
:arrow_down: |
python-pypy-3.7 | 93.58% <100.00%> (-0.09%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
...ecisionengine/framework/logicengine/LogicEngine.py | 96.55% <ø> (-0.12%) |
:arrow_down: |
src/decisionengine/framework/modules/Publisher.py | 100.00% <ø> (ø) |
|
src/decisionengine/framework/modules/Source.py | 100.00% <ø> (ø) |
|
...rc/decisionengine/framework/modules/SourceProxy.py | 65.21% <ø> (-0.50%) |
:arrow_down: |
src/decisionengine/framework/modules/Transform.py | 100.00% <ø> (ø) |
|
src/decisionengine/framework/modules/Module.py | 100.00% <100.00%> (ø) |
|
src/decisionengine/framework/dataspace/maintain.py | 98.11% <0.00%> (-0.95%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update d7f4401...39f7430. Read the comment docs.
I think the change you suggest is fine, and a great solution to the problem of having to add a line in every module. I will let Marco comment on it.
Thanks, @goodenou.
I like the idea to use self and reduce the boilerplate. I let Lisa pick if the class is preferable. It may be since the logger is an attribute of the class.
Anyway, if the module name is preferable, it is possible to get that one as well:
self.logger = self.logger.bind(class_module=type(self).__module__.split(".")[-1], )
Thanks, @mambelli, for your comments. @goodenou, do you prefer the class name or the module name in the bind
statement? Just let me know and I'll adjust the code.
Class name seems a bit better to me.
Thanks, @goodenou--the current PR uses class_name
. I will open a companion PR in decisionengine_modules.
@knoepfel Will you also please remove the following lines from the LogicEngine.py module:
from decisionengine.framework.modules.logging_configDict import CHANNELLOGGERNAME ... self.logger = structlog.getLogger(CHANNELLOGGERNAME) self.logger = self.logger.bind(class_module=name.split(".")[-1], channel=self.channel_name)
It inherits from the Module class and doesn't need these definitions.
Hello @knoepfel! 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! :beers:
Good catch, Lisa. Fixed.
The motivation for this is that in every module that wants to use framework-supported logging, that module must provide the following boilerplate in the module constructor
This PR removes this boilerplate by adding the following in only the framework's
Module
constructor:In many cases, the name of the Python module and the name of the class are the same, so this would not change those cases. This works because the
self
argument is the type of the most derived class that invokessuper().__init__(...)
in its own module constructor.Question: Is this simplification desired despite the slight change in behavior?