-
Notifications
You must be signed in to change notification settings - Fork 94
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
Enhance error reporting and handling for large JSON validations #505
base: main
Are you sure you want to change the base?
Enhance error reporting and handling for large JSON validations #505
Conversation
@pascalrosenberger / @ManbirP / @hirschsebastian could please review the PR, before I will change the status from draft to finish? |
logging.info("Validating JSON") | ||
for json_obj in json_stream: | ||
|
||
def get_primary_key_field(schema: dict, model_name: str) -> str: |
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.
Since primary_key_field
can be theoretically None, I suggest to set as a return type Optional[str].
return primary_key_field | ||
|
||
|
||
def get_primary_key_value(schema: dict, model_name: str, json_object: dict) -> str: |
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.
Since the return value can be theoretically None, I suggest to set as a return type Optional[str].
model=exception.model, | ||
engine=exception.engine, | ||
message=exception.message, | ||
) |
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.
Since we use a property message
here, I suggest to add the property to the Check class as Optional[str].
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.
In three parts of the code I suggest adding Optional[str] to make the possibility of None more explicit. What do you think?
Pull request for issue: #443