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

[FEATURE] Add include_rendered_content to get_expectation_suite and get_validation_result #5853

Merged

Conversation

NathanFarmer
Copy link
Contributor

@NathanFarmer NathanFarmer commented Aug 26, 2022

Changes proposed in this pull request:

  • Add include_rendered_content to context.get_expectation_suite
  • Add include_rendered_content to context.get_validation_result
  • Enable rendering an ExpectationConfiguration not contained in an ExpectationSuite
  • Enable rendering in Checkpoint using DataContext variables
  • Simplify InlineRenderer.get_rendered_content methods now that ExpectationSuites will always be saved or retrieved with rendered_content

Definition of Done

  • My code follows the Great Expectations style guide
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added unit tests where applicable and made sure that new and existing tests are passing.
  • I have run any local integration tests and made sure that nothing is broken.

@netlify
Copy link

netlify bot commented Aug 26, 2022

Deploy Preview for niobium-lead-7998 ready!

Name Link
🔨 Latest commit 88e6ead
🔍 Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/630e14a891d70b000a95cb98
😎 Deploy Preview https://deploy-preview-5853--niobium-lead-7998.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ghost
Copy link

ghost commented Aug 26, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@NathanFarmer NathanFarmer marked this pull request as ready for review August 29, 2022 19:23
@NathanFarmer NathanFarmer changed the title [MAINTENANCE] Add include_rendered_content to get_expectation_suite and get_validation_result [FEATURE] Add include_rendered_content to get_expectation_suite and get_validation_result Aug 29, 2022
@Kilo59 Kilo59 self-requested a review August 30, 2022 01:48
@NathanFarmer NathanFarmer requested a review from Kilo59 August 30, 2022 12:38
Nathan Farmer added 2 commits August 30, 2022 09:44
…' of github.com:great-expectations/great_expectations into feature/GREAT-911/include_rendered_content_on_store_get
@NathanFarmer NathanFarmer requested a review from Kilo59 August 30, 2022 13:46
@Kilo59 Kilo59 requested a review from a team August 30, 2022 13:51
Copy link
Member

@anthonyburdi anthonyburdi left a comment

Choose a reason for hiding this comment

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

Looks good, just one question

"render_object": self,
}
module_name = "great_expectations.render.renderer.inline_renderer"
inline_renderer = instantiate_class_from_config(
Copy link
Member

Choose a reason for hiding this comment

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

Could we just instantiate from the constructor e.g. inline_renderer = InlineRenderer(render_object=self)? Or is this a point of extensibility where we would accept a config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the issue with circular imports that we spent a lot of time trying to solve about a month ago and landed on this as a solution for now.

Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding a small comment here in the code with that explanation? That way if we fix the circular import issue in the future we will have a hint that we can safely remove the instantiate_class_from_config() call

@NathanFarmer NathanFarmer enabled auto-merge (squash) August 30, 2022 14:12
@NathanFarmer NathanFarmer merged commit 2b1a071 into develop Aug 30, 2022
@NathanFarmer NathanFarmer deleted the feature/GREAT-911/include_rendered_content_on_store_get branch August 30, 2022 14:58
Shinnnyshinshin pushed a commit that referenced this pull request Aug 31, 2022
…ssed-in-as-batchspec-passthrough-spark

* develop:
  [MAINTENANCE] increase the `pytest-timeout` timeout value during unit-testing step (#5884)
  [BUGFIX] JSON Serialize RowCondition and MetricBundle computation result to enable IDDict.to_id() for SparkDFExecutionEngine (#5883)
  [MAINTENANCE] new invoke test flags (#5880)
  [MAINTENANCE] Label tests in `tests/core` with `@pytest.mark.unit` and `@pytest.mark.integration` (#5879)
  [MAINTENANCE] add `pytest-timeout` (#5857)
  [BUGFIX] Ensure that `delete_expectation_suite` returns proper boolean result (#5878)
  [FEATURE] Add `include_rendered_content` to `get_expectation_suite` and `get_validation_result` (#5853)
  [MAINTENANCE] run unit-tests on a target package (#5869)
  Update the 'Show DataFrame to SQL times' step of expectation_gallery pipeline to not die if no SQL backend used (#5874)
  don't force using the stand alone mock (#5871)
  [MAINTENANCE] Remove vendored `marshmallow__shade` (#5866)
  [MAINTENANCE] Misc cleanup of `test_expectation_suite_crud.py` (#5868)
  [MAINTENANCE] Handle Pandas warnings in Data Assistant plots (#5863)
  [MAINTENANCE] Update `ExpectationSuite` CRUD on `DataContext` to recognize Cloud ids (#5836)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants