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

[MAINTENANCE] DataContextVariableKey for use in Stores #5255

Merged

Conversation

cdkini
Copy link
Member

@cdkini cdkini commented Jun 7, 2022

Changes proposed in this pull request:

  • Update to DatasourceStore to utilize a new key called DataContextVariableKey
  • This key adheres to a particular schema and allows devs to specify both the resource type and the name of that resource (as applicable). It provides more flexibility than the existing StringKey that was previously used.

Definition of Done

Please delete options that are not relevant.

  • 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.

Thank you for submitting!

@netlify
Copy link

netlify bot commented Jun 7, 2022

Deploy Preview for niobium-lead-7998 ready!

Name Link
🔨 Latest commit faea720
🔍 Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/629f97add446cd00076284f4
😎 Deploy Preview https://deploy-preview-5255--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 Jun 7, 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

Comment on lines +84 to +91
class DataContextVariableKey(DataContextKey):
def __init__(
self,
resource_type: DataContextVariableSchema,
resource_name: Optional[str] = None,
) -> None:
self._resource_type = resource_type
self._resource_name = resource_name
Copy link
Member Author

Choose a reason for hiding this comment

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

Some DataContextVariables are strings or other single values while other ones are dictionaries.

If I want to use my InlineStoreBackend to add a new datasource or store, I want the ability to specify my type (i.e. datasource or store) as well as the name associated with the new object (i.e. my_new_datasource or my_new_store).

The old StringKey did not allow for this flexibility.

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.

Thank you for this PR 😄 Just a few small items to address.

scripts/check_docstring_coverage.py Show resolved Hide resolved
from great_expectations.data_context.types.base import DataContextConfig
from great_expectations.data_context.types.data_context_variables import (
DataContextVariableSchema,
)
from great_expectations.exceptions.exceptions import StoreBackendError
from great_expectations.util import filter_properties_dict

Copy link
Member

Choose a reason for hiding this comment

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

I know this code wasn't modified in this PR, and can be done in a different PR but we should have a docstring for the class that describes what an InlineStoreBackend is. Including mentioning that it is only for use with file-backed data contexts (DataContext and FileDataContext).

Copy link
Member Author

Choose a reason for hiding this comment

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

I definitely agree - should have added this during my initial implementation!

Updated but please let me know if you think anything else should be added.

@cdkini cdkini requested a review from anthonyburdi June 7, 2022 18:22
@cdkini cdkini enabled auto-merge (squash) June 7, 2022 18:24
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.

LGTM!

@cdkini cdkini merged commit 9025c11 into develop Jun 7, 2022
@cdkini cdkini deleted the feature/great-889/great-937/data-context-variable-key branch June 7, 2022 20:07
aezomz pushed a commit to aezomz/great_expectations that referenced this pull request Jun 8, 2022
…tations#5255)

* feat: initial commit

* fix: fix retrieval bug due to substitution

* feat: use enum instead of strings

* chore: update linter thresholds

* chore: respond to initial round of comments

* test: write integration test
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.

2 participants