-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Call get_setting with setting.name #4000
Conversation
and report if someone is calling with non-str argument
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.
Thanks @gamboz, one comment in the review.
src/utils/setting_handler.py
Outdated
@@ -91,6 +91,13 @@ def get_setting( | |||
:default: If True, returns the default SettingValue when no journal specific | |||
value is present | |||
""" | |||
if not isinstance(setting_name, str): | |||
import inspect |
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 the import statement be shifted to the header?
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.
Moved.
I wasn't sure if you wanted to keep this sort of workaround, so I kept everything close for easier deletion 🙂
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.
Thanks gamboz--it is helpful to have the warning for sure. We can remove it when / if we put in Python type annotations, or if we add unit tests to the functions that call get_setting
, with assertions like Mock.assertCalledWith
.
If we want to make it clear it is temporary, maybe we could add a comment about that fact?
done |
Sometimes the function
utils.settings_handler.get_setting()
is called with a wrong argument.The function expects a string as second argument (
setting_name
), but here and there it is called with acore.Setting
object instead. I think that everything works fine anyway becausestr(Setting)
isSetting.name
, but I thought I should mention 🙂Here I fix a couple of calls, and I add (temporary) code to report if someone is calling with non-str argument.