-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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] Clean up public api report part 3 #6803
[MAINTENANCE] Clean up public api report part 3 #6803
Conversation
👇 Click on the image for a new way to code review
Legend |
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Looks good overall! A couple of questions and some opinion base changes.
IncludeExcludeDefinition( | ||
reason="False match for context.get_validator()", | ||
name="get_validator", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/parameter_builder/parameter_builder.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="Not used directly but from Data Assistant or RuleBasedProfiler", | ||
name="run", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/rule/rule.py" | ||
), | ||
), |
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.
Are these needed for the Data Assistant contribution process?
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 don't see either Rule.run()
or ParameterBuilder.get_validator()
used in DataAssistant, OnboardingDataAssistant or the VolumeDataAssistant. It looks like it gets called internally.
Could you point me to the Data Assistant contribution process to double check? I haven't been able to find it in our docs.
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, it doesn't exist yet! Though it is on the roadmap. Perhaps @alexsherstinsky can help answer this? Otherwise we can always add this to the Public API when it's needed.
scripts/public_api_report.py
Outdated
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/checkpoint/types/checkpoint_result.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path("great_expectations/core/batch.py"), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path("great_expectations/core/domain.py"), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/core/expectation_configuration.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/core/expectation_diagnostics/expectation_diagnostics.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path("great_expectations/core/expectation_suite.py"), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/core/expectation_validation_result.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="Internal use", | ||
name="IDDict", | ||
filepath=pathlib.Path("great_expectations/core/id_dict.py"), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path("great_expectations/core/run_identifier.py"), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path("great_expectations/data_context/types/base.py"), | ||
), |
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 these should be public. They are used for passing things around in the Airflow Operator, and some users use it when they need to pass around various forms of config. My hope is that the need for it goes away when we transition to New Style Datasources, but we’re not quite there yet.
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.
Updated (not all, see 5be5c70 for specifics)
scripts/public_api_report.py
Outdated
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/attributed_resolved_metrics.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path("great_expectations/rule_based_profiler/builder.py"), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/config/base.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/data_assistant_result/data_assistant_result.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/estimators/numeric_range_estimation_result.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/estimators/numeric_range_estimator.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/helpers/cardinality_checker.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/helpers/configuration_reconciliation.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/helpers/runtime_environment.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/parameter_container.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/rule/rule.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/rule_based_profiler.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/rule_based_profiler/rule_based_profiler_result.py" | ||
), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path("great_expectations/types/attributes.py"), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path("great_expectations/types/base.py"), | ||
), |
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 above. I don't think all to_json_dict
methods need to be public, but I think any classes that are public, and/or whose config objects are public, should have public to_json_dict
methods.
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.
Updated (see 5be5c70 for specifics)
scripts/public_api_report.py
Outdated
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path("great_expectations/validator/exception_info.py"), | ||
), | ||
IncludeExcludeDefinition( | ||
reason="to_json_dict is an internal helper method", | ||
name="to_json_dict", | ||
filepath=pathlib.Path( | ||
"great_expectations/validator/metric_configuration.py" | ||
), | ||
), |
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 above.
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.
Updated (not all, see 5be5c70 for specifics)
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! Thanks for the updates. For the RBP stuff, we can always add this to the Public API when we're ready for the DA contribution docs, so no need to gate on that.
…ttps://github.com/great-expectations/great_expectations into m/dx-21/dx-157/suppress-warnings-for-result-format * 'm/dx-21/dx-157/suppress-warnings-for-result-format' of https://github.com/great-expectations/great_expectations: (57 commits) [MAINTENANCE] Use shortened_dotted_paths in API docs (#6820) [MAINTENANCE] Use Enum classes for all metric name suffixes (#6819) [BUGFIX]: expect_day_count_to_be_close_to_equivalent_week_day_mean (#6811) [CONTRIB] Improve contrib schwifty expectations (#6812) Add docstring for `AbstractDataContext.add_checkpoint` (#6728) [BUGFIX] Implemented lowercase function to check what type of file endswith (#6810) [DOCS] ExpectationSuite and remove_expectation api docs (#6785) [MAINTENANCE] Clean up public api report part 3 (#6803) [MAINTENANCE] Fixes for column_values_to_be_between tests (#6809) [MAINTENANCE] Improve type hints in ExecutionEngine.resolve_metrics() flow and delete unnecessary checks (#6804) [BUGFIX] Fix missing not operator ~ (#6808) Suppress 2 kl_divergence datasets for bigquery that took 90 minutes to insert (#6807) [MAINTENANCE] Use environment variables for expectation gallery data paths (#6805) [MAINTENANCE] Add checkpoint and datadoc integration test for zep pandas datasource. (#6793) [DOCS] Link to gh issue #4152 for ruamel.yaml (#6799) [MAINTENANCE] Misc docstrings in `AbstractDataContext` (#6801) [MAINTENANCE] Shift daily gallery run by 6 hours (#6802) [MAINTENANCE] Clean up public api report part 2 (#6792) [MAINTENANCE] Clean up public api report part 1 (#6784) [MAINTENANCE] Only include relevant diagnostics info in gallery JSON (#6797) ...
Changes proposed in this pull request:
invoke public-api
invoke public-api
, or look at the tracker with rationales.This PR builds on #6784 and #6792.
Definition of Done