Skip to content
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

Merged
merged 1 commit into from
Sep 24, 2021
Merged

feat: add logger #14

merged 1 commit into from
Sep 24, 2021

Conversation

marvinmarnold
Copy link
Contributor

Why
It is helpful to have verbose logs that are consistent across all services.

How
This logger has been copied from hm-config.

@marvinmarnold marvinmarnold force-pushed the marvinmarnold/add-logger branch from 91a5a25 to fa3e93d Compare September 17, 2021 23:27
Copy link
Contributor

@vpetersson vpetersson left a 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
Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

@vpetersson
Copy link
Contributor

LGTM, but i think you need to merge in master and bump up the version.

@marvinmarnold marvinmarnold force-pushed the marvinmarnold/add-logger branch 3 times, most recently from db37e8a to 7cf3465 Compare September 24, 2021 00:08
It is helpful to have verbose logs. This logger has been
copied from hm-config because all services should use
the same format.
@marvinmarnold marvinmarnold force-pushed the marvinmarnold/add-logger branch from 7cf3465 to 1945b8d Compare September 24, 2021 02:04
@@ -3,7 +3,7 @@

setup(
name='hm_pyhelper',
version='0.6.1',
version='0.6.3',
Copy link
Contributor Author

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 marvinmarnold merged commit 9f3ce5d into master Sep 24, 2021
@marvinmarnold marvinmarnold deleted the marvinmarnold/add-logger branch September 24, 2021 02:11
@vpetersson
Copy link
Contributor

@marvinmarnold FYI - to release you need to create a git tag for the release (e.g. v0.6.3), push this and then generate a release in GitHub. The new release will then be built and pushed to PyPi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants