MNT: improve error messages on bad pdf metadata input #17935
Conversation
d2be6c2 to
1970ce3
Compare
|
I simplified the setup a little, and added tests. |
1970ce3 to
d05d9b3
Compare
d05d9b3 to
6528697
Compare
| @@ -183,12 +183,15 @@ def _create_pdf_info_dict(backend, metadata): | |||
| info = {k: v for (k, v) in info.items() if v is not None} | |||
|
|
|||
| def is_string_like(x): | |||
| """an instance of str""" | |||
There was a problem hiding this comment.
| """an instance of str""" | |
| """Return whether *x* is an instance of ``str``.""" |
There was a problem hiding this comment.
No, the point of the docstring is to be used in the error message. It's a nested function in a private method, so is not otherwise user-facing.
There was a problem hiding this comment.
Oh! In that case, please add a comment below the docstring. Otherwise somebody will "clean" this up later. I'm also wondering why flake8-docstrings is not complaining about the non-compliant docstring format.
Alternatively, It may be better to add the text explicitly to the keywords mapping alongside the checking function than misusing the docstring. Or add an extra attribute to the functions.
There was a problem hiding this comment.
Thinking about it, using the docstring as a message is too clever.
We should instead use an explicit attribute.
def is_string_like(x):
return isinstance(x, str)
is_string_like.expected_value = "an instance of str"
| return isinstance(x, str) | ||
|
|
||
| def is_date(x): | ||
| """an instance of datetime.datetime""" |
There was a problem hiding this comment.
| """an instance of datetime.datetime""" | |
| """Return whether *x* is an instance of ``datetime.datetime``.""" |
| return isinstance(x, datetime) | ||
|
|
||
| def check_trapped(x): | ||
| """one of {"True", "False", "Unknown"}""" |
There was a problem hiding this comment.
| """one of {"True", "False", "Unknown"}""" | |
| """Return whether *x* is one of {"True", "False", "Unknown"}.""" |
Tell the user what keys and types are expected.
6528697 to
b187215
Compare
| @@ -183,12 +183,15 @@ def _create_pdf_info_dict(backend, metadata): | |||
| info = {k: v for (k, v) in info.items() if v is not None} | |||
|
|
|||
| def is_string_like(x): | |||
| """an instance of str""" | |||
There was a problem hiding this comment.
Thinking about it, using the docstring as a message is too clever.
We should instead use an explicit attribute.
def is_string_like(x):
return isinstance(x, str)
is_string_like.expected_value = "an instance of str"
PR Summary
Tell the user what keys and types are expected.
This still needs tests. Happy if anyone wants to adopt the PR and finish it.
PR Checklist