ERR: Fix missing space in warning message #39159
Conversation
There was a problem hiding this comment.
Thanks @MicaelJarniac for the PR.
We normally check error messages in the tests so we would normally expect that changing a message would also require a change in the tests.
so it maybe that there are no tests that hit this or the message is only partially checked. so further investigation welcome.
| @@ -1991,7 +1991,7 @@ def _concatenate_chunks(list chunks): | |||
| if warning_columns: | |||
| warning_names = ','.join(warning_columns) | |||
| warning_message = " ".join([ | |||
There was a problem hiding this comment.
does removing the join here work instead and just use fstring literal concatenation
There was a problem hiding this comment.
I can't see why that wouldn't work. Technically, the join wasn't even being used anyways, because the strings were getting concatenated inside of the list, so yeah, "officially" removing the join and manually adding the space between the strings sounds perfectly fine I think.
I'll poke around to see how other similar warnings are created, though, because keeping it consistent between them might be a valid reason as to whether or not to have the join.
simonjayhawkins
added
the
Error Reporting
simonjayhawkins
changed the title
I'll try locating the tests related to this specific message (if any), and fix (or add) them. And I'll also try to think of a way for catching similar issues in future tests, but I don't think it'd be easy, as simply saying "fail if no space after period" would catch this one, but would probably have about a thousand false-positives in other messages, because in Python syntax it's perfectly valid to have |
it appears that test_warn_if_chunks_have_mismatched_type[c_low] hits this. so can modify that test to also check message. |
simonjayhawkins
added
Warnings
pytest pandas/tests/io/parser/common/test_chunksize.py::test_warn_if_chunks_have_mismatched_type
|
|
| if record: | ||
| assert ( | ||
| str(record[0].message) | ||
| == "Columns (0) have mixed types. Specify dtype option on import or " |
There was a problem hiding this comment.
The string is quite long, so it doesn't look particularly pretty in here, but I think it's okay-ish.
There was a problem hiding this comment.
usually we split this into two lines
expected = "Columns (0) ..."
assert str(record[0].message) == expected
|
@MicaelJarniac can you merge master and we'll try to get this in |
Co-authored-by: Simon Hawkins <[email protected]>
|
closing as stale. @MicaelJarniac if you'd like to rebase and fix this up pls ping. |
Uh oh!
There was an error while loading. Please reload this page.