Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] V1 Checkpoint #9590
[FEATURE] V1 Checkpoint #9590
Changes from 24 commits
44546e2
d93bd88
fe25879
452953f
ecbba25
df5b13f
21ef021
0435290
97b2106
e0b9a30
b32079b
198427a
a0023fb
29ac1a9
bfc2855
ffc8ec0
479f1ff
f4aac67
4a2bd89
aa53728
092c9fe
9f18740
10c12c1
b55d802
12f69b9
50da4c2
36d1ea9
d047ac4
e5c072d
571f245
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why not a list of
ValidationDefinitions
instead of config objects? That's what this shows: https://greatexpectations.atlassian.net/wiki/spaces/SUP/pages/917471267/Validation+WorkflowsOr does the ValidationConfig need to be renamed?
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.
Oh, I think ValidationConfig does need to be renamed. That can be done in a separate PR.
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.
Needs to be renamed!
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.
Do we need this
if
? With the try/except above, it looks like this will need to be defined.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.
did it to appease
mypy
but updated to be a bit clearerThere 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 would expect a method called
serialize
on an object to serialize the object but this looks like it serializes the validation definition? Could you update the name?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 to
serialize_validation_definition
!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.
Moved any encoding logic to the actual class being serialized (move from
_encode_suite
in validation config toserialize
as instance method on suite).I don't love exposing these publicly but how do we get around this? Do we want custom serializer classes to encapsulate this logic?
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.
This only serializes this object when it is used in another object right? I would call this
serialize_identifier_bundle()
or justidentifier_bundler()
. This name should be uniform across any object that needs to do this. I think this public since whatever object serializes this expects this method to exist. We could formalize it as an internal protocol.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 agree for suites and validation definitions but renderers and batch definitions have different outputs (
_EncodedValidationData
andclass_name/module_name
, respectively).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.
Shared space with intermediate serialization models
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.
Same comment as before. I would rename to
identifer_bundle()
.