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] F/great 1463/add updates with datasource obj #7401

Merged
merged 10 commits into from Mar 22, 2023

Conversation

billdirks
Copy link
Contributor

@billdirks billdirks commented Mar 17, 2023

This allows one to use the datasource crud methods with either a datasource object or a set of constructor arguments. Previously only a set of constructor arguments was possible. This is useful because one way add assets or otherwise modify a datasource but that won't be saved to config until the user calls an update on the datasource.

The main complication I ran into was the previously allowed the name to be passed in as a positional or keyword argument. To maintain this or allow the datasource to be passed in positionally I had to do some extra validation to make sure the arguments were sensible.

  • 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 Mar 17, 2023

Deploy Preview for niobium-lead-7998 canceled.

Name Link
🔨 Latest commit 9fb9b79
🔍 Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/641a3c0efc83570008142bea

@ghost
Copy link

ghost commented Mar 17, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@billdirks
Copy link
Contributor Author

Before this PR is merged I need to:

  1. Update the stub file
  2. Rename the parameter first to name_or_datasource

@billdirks billdirks marked this pull request as draft March 18, 2023 06:09
Copy link
Member

@Kilo59 Kilo59 left a comment

Choose a reason for hiding this comment

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

I'm approving but I do think that the stubs should be updated before merging.
As it could expose typing or issues with auto-completion.

Also non-blocking but I think single-dispatch might make this implementation simpler and reduce the number of conditionals.
https://github.com/great-expectations/great_expectations/pull/7401/files#r1143332512

billdirks and others added 2 commits March 21, 2023 15:02
@billdirks
Copy link
Contributor Author

I've updated the stub files and renamed first to name_or_datasource.

@billdirks billdirks marked this pull request as ready for review March 21, 2023 22:19
@billdirks billdirks enabled auto-merge (squash) March 21, 2023 22:20
@billdirks billdirks disabled auto-merge March 21, 2023 23:43
@billdirks billdirks merged commit a5e5ebf into develop Mar 22, 2023
34 checks passed
@billdirks billdirks deleted the f/GREAT-1463/add-updates-with-datasource-obj branch March 22, 2023 00:55
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

2 participants