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

Add logging example code for when create_cli==no #39

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MaxPensel
Copy link

Extended files:

{{cookiecutter.project_slug}}/src/{{cookiecutter.module_name}}/main.py
{{cookiecutter.project_slug}}/notebooks/example.ipynb

Affected option: config_file

Chosing hocon option creates the following code in main.py:

import logging
from test_hocon import util

logger = logging.getLogger("test_hocon")


def main():
    config = util.load_config()
    util.logging_setup(config)

    logger.info("Looks like you're all set up. Let's get going!")

    # TODO your journey starts here
    print("hello :)")


if __name__ == "__main__":
    main()

and adds this cell content to example.ipynb:

import logging
from test_hocon import util

logger = logging.getLogger("test_hocon")

config = util.load_config()
util.logging_setup(config)

logger.info("Use this setup to start logging in notebooks or to "+
            "get the correct logging format in your test_hocon module")

Chosing yaml option creates the following code in main.py:

import os
import logging
from test_yaml import util

logger = logging.getLogger("test_yaml")


def main():
    config = util.load_config(os.path.join(os.path.abspath(__file__),
                                           *[os.pardir]*3,
                                           "config",
                                           "config.yml"))
    util.logging_setup(config)

    logger.info("Looks like you're all set up. Let's get going!")

    # TODO your journey starts here
    print("hello :)")


if __name__ == "__main__":
    main()

and adds this cell content to example.ipynb:

import os
import logging
from test_yaml import util

logger = logging.getLogger("test_yaml")

config = util.load_config(os.path.join(os.path.abspath(os.pardir),
                                       "config",
                                       "config.yml"))
util.logging_setup(config)

logger.info("Use this setup to start logging in notebooks or to "+
            "get the correct logging format in your test_yaml module")

The path construction to the config.yml may not be ideal, but could be fixed when addressing issue #38 .

Sensitive to logging option (yaml/hocon)
Add cell in example.ipynb and sample code in main.py
@MaxPensel
Copy link
Author

In reference to the issue mentioned in PR #26, should python files containing cookiecutter syntax be excluded in .pre-commit-config.yaml?
Otherwise the check-ast pre-commit hook will always fail parsing .py files as correct python syntax.

@klamann
Copy link
Contributor

klamann commented Feb 26, 2021

I adjusted main.py because I had a few issues with it: discovery of the config file was too complex, just providing a static path should be enough to get you started, and hocon or having no config was not a well-supported case.

I'm still not sure about this PR, because we're adding a lot of complexity here that many of our users might not care for.

If you want to continue with this, can you adjust the notebook code in a similar fashion?

@klamann
Copy link
Contributor

klamann commented Feb 26, 2021

ok, now I'm also not happy with the static config path, because it probably won't work by default for most users (e.g. if you launch it in PyCharm, it will try to find the config relative to main.py and not the project root folder).

well... I don't know. If we have the CLI, we can let the user define the path to the config, but in this case, I don't think we're setting a good example by defining a path in the code where the config has to be.

@MaxPensel
Copy link
Author

Yeah, that is the reason why I opted to build the relative path from main.py to the config using os.

I agree that fixing a path in the library is not good design, I thought of it more as an example, so that people would get a hint at how to make use of logging. In that line of reasoning, it would still make sense to have a static path in the notebook, because that is not part of the python package that is being built, and notebooks are always executed from where they are, so the relative path from example.ipynb to the config should work in any setup.

I also agree that complexity should be kept low, but at the same time we want to encourage good design, which includes logging instead of printing wherever possible. My first experience with the template was just that it didn't paint a clear picture of how to utilize the logger correctly.

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