-
Notifications
You must be signed in to change notification settings - Fork 0
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
239 have a missing config parser #245
base: master
Are you sure you want to change the base?
Conversation
setattr(config, key, getattr(config_template, key)) | ||
self.error_count += 1 | ||
else: | ||
if type(getattr(config_template, key)) != type(getattr(config, key)) and type(getattr(config_template, key)) != type(None): |
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 think isinstance()
should be used here instread of the type != type.
ALso the getattr()
will raise an AttributeError
if you dont pass in a default and the attribute is not found.
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.
if not isinstance(getattr(config, key, None), type(getattr(config_template, key, None))) and getattr(config_template, key, None) is not None:
would be more pythonic and remove the AttributeError issue.
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.
converts to review comment
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.
Am I missing something? The first IF checks if the config item exists at all, the second IF under the ELSE (item must exist) you've called out here is specifically to identify incorrect types based on the example config type.
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.
Ah right I see.
I still think using isinstnace
and using is
to compare NoneType
values is the pythonic way to go.
if not isinstance(getattr(config, key), type(getattr(config_template, key))) and getattr(config_template, key) is not None:
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.
if not isinstance(getattr(config, key), type(getattr(config_template, key))) and getattr(config_template, key) is not None:
...
(forgot to format it in my previous comment)
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.
See comments
setattr(config, key, getattr(config_template, key)) | ||
self.error_count += 1 | ||
else: | ||
if type(getattr(config_template, key)) != type(getattr(config, key)) and type(getattr(config_template, key)) != type(None): |
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.
converts to review comment
No description provided.