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

239 have a missing config parser #245

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sjefferson99
Copy link
Member

No description provided.

@sjefferson99 sjefferson99 linked an issue Oct 10, 2024 that may be closed by this pull request
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):
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

converts to review comment

Copy link
Member Author

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.

Copy link
Collaborator

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:

Copy link
Collaborator

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)

Copy link
Collaborator

@sam57719 sam57719 left a comment

Choose a reason for hiding this comment

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

See comments

smibhid/lib/config/config_management.py Show resolved Hide resolved
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):
Copy link
Collaborator

Choose a reason for hiding this comment

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

converts to review comment

smibhid/README.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have a missing config parser
2 participants