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: A way to warn user on default variable usage #454

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

panteparak
Copy link

This PR added a way to warn users of any variable that is using their default values.

@github-actions
Copy link

Your PR was set to target main, PRs should be target develop
The base branch of this PR has been automatically changed to develop, please check that there are no merge conflicts.

@github-actions github-actions bot changed the base branch from main to develop March 22, 2023 13:28
Copy link
Collaborator

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea because it is a popular trap - not the obvious use of defaults. However, I would ask you to rethink your use of the logging. Also, could you please amend the tests and write a short note in the change log. Thank you!

@@ -391,6 +394,8 @@ def get_value(self, var, cast=None, default=NOTSET, parse_default=False):
raise ImproperlyConfigured(error_msg) from exc

value = default
if self._WARN_ON_DEFAULT_VALUE_USAGE:
logger.warn(f"Variable '{var}' is using a default variable.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a tutorial-style HOWTO taking you through the steps in using the logging module. https://docs.python.org/3/howto/logging.html

warnings.warn() in library code if the issue is avoidable and the client application should be modified to eliminate the warning

logging.warning() if there is nothing the client application can do about the situation, but the event should still be noted

Considering the above, do you still think that the logging is the better solution here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it again, I have to agree with you. the pythonic warning package is a better substitute for this case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please made the necessary changes?

@sergeyklay sergeyklay self-assigned this Mar 22, 2023
@sergeyklay sergeyklay added enhancement New feature or request good first issue Good for newcomers labels Mar 22, 2023
@coveralls
Copy link

Coverage Status

Coverage: 90.96% (-0.3%) from 91.255% when pulling 0e6bc76 on panteparak:main into 1e834a1 on joke2k:develop.

@coveralls
Copy link

Coverage Status

Coverage: 91.149% (-0.1%) from 91.255% when pulling 0e6bc76 on panteparak:main into 1e834a1 on joke2k:develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants