gh-137899: Enhance shelve serializer validation with descriptive error messages#138768
gh-137899: Enhance shelve serializer validation with descriptive error messages#138768furkanonder wants to merge 10 commits intopython:mainfrom
Conversation
furkanonder
added
type-feature
| self.deserializer = deserializer | ||
|
|
||
| @staticmethod | ||
| def _validate_serialized_value(serialized_value, original_value): |
There was a problem hiding this comment.
Validation is also at the level of the C extension (in dbmmodule.c) so you should also change the message out there.
furkanonder
requested review from
corona10,
erlend-aasland and
serhiy-storchaka
as code owners
|
I've sent some suggestions as a PR to your branch: furkanonder#3 |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I am not sure this is the best solution of the original issue. Making error messages in the underlying database more specific would help too.
| msg = (f"Serializer returned {invalid_type} for value " | ||
| f"{original_value!r} But database values must be " | ||
| f"bytes or str, not {invalid_type}") |
There was a problem hiding this comment.
invalid_type is repeated twice. This is unnecessary.
The repr of original_value can be very large, it should not be included in the error message. Error most likely does not depend on the original value.
Improve dbm/gdbm error messages
Thanks a lot for the PR! |
I'm not entirely clear on the specific approach you're suggesting. Could you provide an example of what you have in mind? This would help users immediately identify which underlying database implementation is involved in the error. Would this address your concern about making the error messages in the underlying database more specific, or were you thinking of a different approach? |
|
This PR is stale because it has been open for 30 days with no activity. |
6 participants
shelvedatum #137899