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] Ensure that id_ fields in Marshmallow schema serialize as id #5660

Merged
merged 16 commits into from
Aug 3, 2022

Conversation

cdkini
Copy link
Member

@cdkini cdkini commented Aug 3, 2022

Changes proposed in this pull request:

  • Ensure that all cases of id_ in our Marshmallow schema properly serialize as id

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 run any local integration tests and made sure that nothing is broken.

@netlify
Copy link

netlify bot commented Aug 3, 2022

Deploy Preview for niobium-lead-7998 ready!

Name Link
🔨 Latest commit 16e29d8
🔍 Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/62eada5b9b7dd4000826abba
😎 Deploy Preview https://deploy-preview-5660--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 3, 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

@cdkini cdkini marked this pull request as draft August 3, 2022 12:02
@cdkini cdkini changed the title refactor: update Marshmallow schemas [MAINTENANCE] Ensure that id_ fields in Marshmallow schema serialize as id Aug 3, 2022
@@ -2239,8 +2240,27 @@ class CheckpointValidationConfigSchema(Schema):
class Meta:
unknown = INCLUDE

name = fields.String(required=False, allow_none=False)
Copy link
Member Author

Choose a reason for hiding this comment

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

Should have been removed in a prior PR but forgot to.

Comment on lines +2334 to +2336
cls_or_instance=fields.Nested(CheckpointValidationConfigSchema),
required=False,
allow_none=True,
Copy link
Member Author

Choose a reason for hiding this comment

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

Never converted this over to the typed obj - thanks @Kilo59!

@cdkini cdkini self-assigned this Aug 3, 2022
@cdkini cdkini marked this pull request as ready for review August 3, 2022 14:42
id_ = fields.String(required=False, allow_none=False)
id_ = fields.String(required=False, allow_none=False, data_key="id")

def dump(self, obj: dict, *, many: Optional[bool] = None) -> dict:
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have an explicit list of what can go in validations nested within checkpoints? If so, we can get rid of this. As this has historically been a dictionary, I wanted to guard against edge cases and make this as forgiving of unknown values as possible.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Also take a look at get_substituted_validation_dict().
Maybe best to create the list but also leave it open until we can make sure we have them all.

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 enabled auto-merge (squash) August 3, 2022 15:20
@cdkini cdkini merged commit 36283ec into develop Aug 3, 2022
@cdkini cdkini deleted the maintenance/rename-id_-to-id-when-serializing-objs branch August 3, 2022 22:06
Shinnnyshinshin pushed a commit that referenced this pull request Aug 5, 2022
…etric-change

* develop: (30 commits)
  Add mysql client to Dockerfile.tests (#5681)
  [DOCS]added guide how to use great expectations with aws glue (#5536)
  [MAINTENANCE] Assign `resource_type` in `InlineStoreBackend` constructor (#5671)
  [RELEASE] 0.15.17 (#5675)
  fix: patch issue with 'id' vs 'id_' (#5677)
  Tidying up  the texts and config in gen; bump ops (#5678)
  [MAINTENANCE] data_context initial type checking (#5662)
  [FEATURE]Enable showing expectation suite by domain and by expectation_type -- from DataAssistantResult (#5673)
  [MAINTENANCE] Ensure that `id_` fields in Marshmallow schema serialize as `id` (#5660)
  [MAINTENANCE] Improve DateTime Conversion Candling in Comparison Metrics & Expectations and Provide a Clean Object Model for Metrics Computation Bundling (#5656)
  [MAINTENANCE] Only run Cloud E2E tests in primary pipeline
  Refactor docker test CI steps into jobs. (#5665)
  [MAINTENANCE] move tool config to pyproject.toml (#5649)
  add expectation for crc32 (#5580)
  Quick bug fix to all profile numeric column bounds expectations (#5651)
  Don't include 'test-pipeline' in extras_require dict (#5659)
  [ANALYTICS] add new invoke tasks to `tasks.py` and create new file `usage_stats_utils.py` (#5593)
  [MAINTENANCE] Introduce running tests in docker on our Azure CI pipeline. (#5646)
  [MAINTENANCE] Add E2E Cloud test for `DataContext.add_checkpoint()` (#5653)
  Make comparisons of aggregate values date aware (#5642)
  ...
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.

None yet

3 participants