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

Replace print statement with logger #187

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

jochasinga
Copy link
Contributor

As per #179

@jochasinga jochasinga marked this pull request as ready for review August 16, 2021 23:02
@jochasinga jochasinga marked this pull request as draft August 16, 2021 23:08
@chester-leung
Copy link
Member

@jochasinga you can auto-fix the linting errors before committing by following step 3 here. We use pre-commit to format everything.

I'm not sure we should be changing the print() statements in the setup.py file to log messages -- this file is special as running the file will install the Python package. We use print() statements to more clearly notify the user of any installation / environment variable modifications. Using a logger may obscure the messaging. @ryanleh what do you think?

@ryanleh
Copy link
Contributor

ryanleh commented Aug 17, 2021

Ya, if we eventually want to push the package to PyPi then I think we'll need to stick with print statements? Not sure how the logger would interact with pip.

Per the linked issue, I think switching to using a logger for the tests making sense, but not the install script.

This reverts commit 853d481.

It's best to leave print statements in the setup script.
See also: mc2-project#179
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.

3 participants