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
[MAINTENANCE] Refactor RuleBasedProfilerConfig
#4882
[MAINTENANCE] Refactor RuleBasedProfilerConfig
#4882
Conversation
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
…_expectations into maintenance/great-464/great-610/refactor-rbp-config-types
…_expectations into maintenance/great-464/great-610/refactor-rbp-config-types
if config.get("class_name") == "RuleBasedProfiler": | ||
config.pop("class_name", None) | ||
config.pop("module_name", 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.
Ugh this is ugly - how do I differentiate between subclasses that want these attrs and those that don't?
…_expectations into maintenance/great-464/great-610/refactor-rbp-config-types
…es' of https://github.com/great-expectations/great_expectations into maintenance/great-464/great-610/refactor-rbp-config-types
…_expectations into maintenance/great-464/great-610/refactor-rbp-config-types
config.pop("class_name", None) | ||
config.pop("module_name", 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.
Overrode the parent method because we need to pop these values. Could this be done in a cleaner fashion?
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.
Love it. Thank you @cdkini
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.
LGTM!
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.
LGTM!
Changes proposed in this pull request:
class_name
andmodule_name
as arguments in the config constructor (as they as consistent between objects)Definition of Done
Please delete options that are not relevant.
Thank you for submitting!