-
Notifications
You must be signed in to change notification settings - Fork 157
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
Json Decoder Code Cleanup #814
Json Decoder Code Cleanup #814
Conversation
micdavis
commented
May 9, 2023
•
edited
edited
- cleaned up json decoder code
profiles = {CategoricalColumn.__name__: CategoricalColumn} | ||
profiles = { | ||
CategoricalColumn.__name__: CategoricalColumn, | ||
BaseColumnProfiler.__name__: BaseColumnProfiler, |
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 we need to have a bigger discussion, but I'm not sure it is appropriate to decode an abstract class. I'm concerned this could cause bugs / problems if it tries to initialize an abstract class.
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 we instead list all the non-abstract classes here, will it be able to load all of them?
|
||
profile_class = profiles.get(class_name) | ||
if profile_class is None: | ||
raise ValueError(f"Invalid profiler class {class_name} " f"failed to load.") | ||
else: | ||
return profile_class(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.
We can remove the else here and move the return over an indent. Raising above would fail otherwise.