-
Notifications
You must be signed in to change notification settings - Fork 163
[fix]:461 fixed with a utility function that checks for booealn inter… #493
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
base: master
Are you sure you want to change the base?
Conversation
…pretation fo string and integer content. Raises error to the user otherwise
python/utils.py
Outdated
| return ''.join(random.choices(string.ascii_letters + string.digits, | ||
| k=length)) | ||
|
|
||
| def boolean_of(input, context: str) -> bool : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also make the function not sensitive to case? For example, "nO" or "trUe" should work as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking to make the users go to a typesafe region over time that's why I raised the error
python/utils.py
Outdated
| elif input in booleanDictionary["trueStatements"]: | ||
| return True | ||
| else | ||
| raise Exception(f"In ${context} you provided ${input} which cannot be considered a boolean in our source-code please use: False : ${booleanDictionary["falseStatements"]} and True: ${booleanDictionary["trueStatements"]}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you instead of exception issue a warning? using the LOGGER?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be LOGGER.warning()
|
how's it now? |
python/utils.py
Outdated
| elif statement.lower() in booleanDictionary["trueStatements"]: | ||
| return True | ||
| else: | ||
| LOGGER.warning(f"In ${context} you provided ${input} which cannot be considered a boolean in our source-code please use: False : ${booleanDictionary["falseStatements"]} and True: ${booleanDictionary["trueStatements"]}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to escape the double quotes ". or use single quotes '.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, can you remove in our source-code from the warning message?
| LOGGER: logging.Logger | ||
|
|
||
| def generate_graph(dframe, args, suffix: str | None = None) -> None: ... | ||
| def boolean_of(): ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The types do no match
The #461 opened the possibility that the user might un-intentionally enter string values instead of boolean for some inputs, resulting in an always
Truecase.I've added a utility function after the discussion with @kjvbrt about it and fixed the related histogram logarithmic scale bug with it as well.