gh-62040: Raise ValueError on invalid *errors* argument in several codecs - #136611
gh-62040: Raise ValueError on invalid *errors* argument in several codecs#136611StanFromIreland wants to merge 7 commits into
ValueError on invalid *errors* argument in several codecs#136611Conversation
StanFromIreland
changed the title
ValueError on invalid *errors* parameter in several codecsValueError on invalid *errors* argument in several codecs
|
I can see that there's only some changes from last patch: For example, I'd like to remove also |
|
Ah I forgot about the patch and my grep was too strict, I'll fix all the self.errors cases. |
|
|
||
| ### Codec Helpers | ||
|
|
||
| def _assert_strict(errors): |
There was a problem hiding this comment.
Maybe _check_strict from the original patch would be better?
There was a problem hiding this comment.
I prefer assert_strict since it is closer to what was there before, and it is more obvious, to me at least, that it will fail if errors != 'strict', whereas check_strict, seems like something that will enable more verbose errors or the like.
There was a problem hiding this comment.
IMO, it's not fully correct to name method assert_something and don't make any assert checks or raising an AssertionError.
There was a problem hiding this comment.
@malemburg which do you prefer? _check_strict or _assert_strict
There was a problem hiding this comment.
I don't have a preference. Both are fine.
This comment was marked as outdated.
This comment was marked as outdated.
|
Little ping @malemburg :-) |
|
This PR is stale because it has been open for 30 days with no activity. |
|
|
||
| ### Codec Helpers | ||
|
|
||
| def _assert_strict(errors): |
There was a problem hiding this comment.
This function is duplicated in six modules. Since all of them already import codecs, would not it be better to add a shared implementation in codecs? Change the name to be clear that it asserts/checks that the error handler is strict only.
There was a problem hiding this comment.
Agreed; this helper should live in codecs.py.
|
|
||
| def _assert_strict(errors): | ||
| if errors != 'strict': | ||
| raise ValueError(f'Unsupported error handling mode: "{errors}" - must be "strict"') |
There was a problem hiding this comment.
"error handling mode" does not occur anywhere in the code or documentation. Only "error handling"/"error handler".
idna raises f"Unsupported error handling: {errors}". This is not a strong precedent, and ValueError is more appropriate than UnicodeError, but take this into account.
There was a problem hiding this comment.
"error handling parameter" would be clearer.
| self.assertRaises(ValueError, encoder, 'in', errors='notstrict') | ||
| self.assertRaises(ValueError, decoder, 'in', errors='notstrict') |
There was a problem hiding this comment.
Input should be bytes, not str.
malemburg
left a comment
malemburg
left a comment
There was a problem hiding this comment.
Please see my last review comment.
|
|
||
| ### Codec Helpers | ||
|
|
||
| def _assert_strict(errors): |
There was a problem hiding this comment.
I don't have a preference. Both are fine.
|
|
||
| ### Codec Helpers | ||
|
|
||
| def _assert_strict(errors): |
There was a problem hiding this comment.
Agreed; this helper should live in codecs.py.
|
|
||
| def _assert_strict(errors): | ||
| if errors != 'strict': | ||
| raise ValueError(f'Unsupported error handling mode: "{errors}" - must be "strict"') |
There was a problem hiding this comment.
"error handling parameter" would be clearer.
|
|
||
| def base64_encode(input, errors='strict'): | ||
| assert errors == 'strict' | ||
| _assert_strict(errors) |
There was a problem hiding this comment.
Hmm, now that I see where you are using the helper, I'm not so sure that this is the right approach anymore.
"assert" only works when Python is run without -O, so the helper does change semantics. I'm also worried that the extra function call will make things slower.
The change in semantics should be documented and I would prefer to have the checks inlined in all these cases. Yes, it's duplication, but those are only two lines of code and the error message could additionally include information about which encoding caused the problem.
|
When you're done making the requested changes, leave the comment: |
|
Note that encoders cannot fail -- every input is valid. The error handler never used, even if it was supported. Why not simply remove assertions in encoders? |
4 participants
assertfor runtime validity checks #62040