-
Notifications
You must be signed in to change notification settings - Fork 8
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
Adding FailedRowProcessor support in soda-spark #114
base: main
Are you sure you want to change the base?
Conversation
- Allowing users to use the FailedRowsProcessor feature by passing it in the execute method
Thanks @joaoluga for the PR. Could you add a test for the failed rows processor? |
hey @JCZuurmond, sorry for taking so long. Yes and I've just included the tests for the failed row processor just now. 😁 |
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 have a couple suggestions
tests/test_scan.py
Outdated
class InMemoryFailedRowProcessor(FailedRowsProcessor): | ||
def process(self, context: dict) -> dict: | ||
|
||
try: |
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 try except does not do anything, right?
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.
Yes, you are correct. I was just following the pattern I found in this doc 😅 Just changed the except
to throw the exception 🤔
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.
LGTM. @vijaykiran could you give a final go?
Thank you @joaoluga and @JCZuurmond - LGTM! |
Resolves #113
snippet:
expected output: