-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: add logger #14
feat: add logger #14
Conversation
91a5a25
to
fa3e93d
Compare
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.
Accepted with minor comment.
README.md
Outdated
To run tests: | ||
|
||
```bash | ||
pip install -r test-requirements |
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.
Needs to be:
pip install -r requirements.txt
pip install -r test-requirements.txt
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.
Thanks, fixed: 9d9d1d8
|
||
```bash | ||
pip install -r test-requirements | ||
PYTHONPATH=./ pytest |
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.
Don't think the explicit PYTHONPATH
is required here.
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.
I'm unsure if this is a best practice but with the code as-is now, you'll get an error if just trying to pytest
. This type of thing is a problem whenever we have the test code in a separate directory/module. In other repos, we get around it by modifying the path from the test code, which I think is worse.
✗ pytest
========================================== test session starts ===========================================
platform linux -- Python 3.7.3, pytest-6.2.5, py-1.10.0, pluggy-1.0.0
rootdir: /home/user/code/nebra/hm-pyhelper
collected 10 items / 1 error / 9 selected
================================================= ERRORS =================================================
________________________ ERROR collecting hm_pyhelper/tests/utils/test_logger.py _________________________
ImportError while importing test module '/home/user/code/nebra/hm-pyhelper/hm_pyhelper/tests/utils/test_logger.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/home/user/.pyenv/versions/3.7.3/lib/python3.7/importlib/__init__.py:127: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
tests/utils/test_logger.py:2: in <module>
from utils.logger import get_logger, _log_format
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.
pwd
should be in PYTHONPATH, but this is not a big deal.
LGTM, but i think you need to merge in master and bump up the version. |
db37e8a
to
7cf3465
Compare
It is helpful to have verbose logs. This logger has been copied from hm-config because all services should use the same format.
7cf3465
to
1945b8d
Compare
@@ -3,7 +3,7 @@ | |||
|
|||
setup( | |||
name='hm_pyhelper', | |||
version='0.6.1', | |||
version='0.6.3', |
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.
This had to be bumped twice because of this: #20
@marvinmarnold FYI - to release you need to create a git tag for the release (e.g. |
Why
It is helpful to have verbose logs that are consistent across all services.
How
This logger has been copied from hm-config.