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

Pre-commit hooks not looking out for print statements #11990

Closed
thesujai opened this issue Mar 15, 2024 · 2 comments · Fixed by #11994
Closed

Pre-commit hooks not looking out for print statements #11990

thesujai opened this issue Mar 15, 2024 · 2 comments · Fixed by #11994
Assignees
Labels
DEV: tools Internal tooling for development P2 - normal Priority: Nice to have TAG: tech update / debt Change not visible to user

Comments

@thesujai
Copy link
Contributor

thesujai commented Mar 15, 2024

Observed behavior

Reference: #11978 (comment)
Currently the precommit hooks are not detecting print statements in our code base, and there are many of them that have made into our codebase.

Expected behavior

  • Pre-commit hooks should fail if any print statements(without noqa comments) are there in the code.(Introduce flake8-print)
  • Wherever there are print statements in codebase, they must be either replaced by logger or noqa comments should be added to them

User-facing consequences

Developers using print statements for testing and debugging miss out some print statements while commiting. So the pre-commit will stop them from doing so.

Steps to reproduce

Run the pre-commit on all files, you can see everything pass, but there are many print statements throughout codebase
https://github.com/learningequality/kolibri/blob/develop/kolibri/core/auth/management/utils.py#L52

Context

Observed on develop but same for elsewhere

@thesujai
Copy link
Contributor Author

I would be happy to raise a PR for this.
Just a confusion where to use noqa and where to use logger

@bjester
Copy link
Member

bjester commented Mar 15, 2024

@thesujai I'll assign you. I think most existing usages of print would have to be migrated to use logging, unless it seems clear it was accidentally added for debugging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: tools Internal tooling for development P2 - normal Priority: Nice to have TAG: tech update / debt Change not visible to user
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants