Fix logging refactor

[Tushar-sketch-bit]

Related Issues / Pull Requests

List all related issues and/or pull requests if there are any.

Description

Changed the logging type from print to logging method as a standard approach protocol against issue #71

What changes are proposed in this pull request?

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality not to work as expected; for instance, examples in this repository must be updated too)
• This change requires a documentation update

Checklist:

• My code modifies existing public API, or introduces new public API, and I updated or wrote documentation
• I have commented my code
• My code requires documentation updates, and I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[Aman-Cool]

@Tushar-sketch-bit, check the lint failures...mainly formatting issues :)

[Aman-Cool]

Hey @Tushar-sketch-bit, appreciate you taking the time to contribute, but after reviewing this PR we won't be moving forward with it.

There are a few fundamental issues, from venv import logger is a broken import that would fail at runtime, the log levels are incorrect throughout, the logger is being initialized inside functions rather than at module level, and a couple of the files mentioned in the issue aren't touched at all. On top of that there are some unrelated changes mixed in that don't belong here.

Rather than trying to patch this one up, I'd encourage you to close it and pick up a fresh issue instead. Take a bit more time to read through the existing code before diving in, understanding how the codebase already handles things like logging goes a long way. There are some good open issues that would be a great fit, happy to point you toward one if you'd like...!

[jeanbez]

Thanks for the input! Just a reminder for everyone: while community feedback is always welcome and helpful, final decisions on PRs are made by the core maintainers.

[Aman-Cool]

Hey @jeanbez, totally fair, just to clarify the reasoning though. The PR has a few blockers that make it not worth patching up: there's a broken import (from venv import logger) that would fail at runtime, incorrect log levels throughout, the logger is initialized inside function bodies instead of at module level, and two of the five files listed in the issue aren't touched at all. On top of that there are unrelated changes mixed in that have nothing to do with the issue.

There's also PR #81 open for the same issue which is in much better shape, and having two contributors working toward the same fix creates unnecessary conflict and review overhead. It made more sense to consolidate around the stronger submission and let @Tushar-sketch-bit pick up a different issue instead.

Happy to discuss if you see it differently!

[kaveenh]

I totally agree with @jeanbez point. PR decisions ultimately rest with the maintainers. We aim to stay open and encouraging to all contributions, while also keeping the review process efficient and manageable on our end.

[kaveenh]

resolved in the latest merge