Repository navigation
Only swallow format errors while probing a database string - #827
Conversation
|
Before I have a deeper look at this, I would like to see all format parsers to only raise |
|
Makes sense — with every loader raising |
it should stay a |
|
I'm too much of a coward to merge this in its current form: I would like to see a additional "catchall" handlers which prints a warning message featuring a traceback instructing people to open a bug report for cantools which includes said traceback. |
a29369d to
5865d98
Compare
| def emit(self, record): | ||
| raise OSError(5, 'Input/output error') | ||
|
|
||
| logger = logging.getLogger('cantools') |
There was a problem hiding this comment.
a comment of how the "unexpected" exception is triggered would be good. AFAICS, you "misappropriate" logging.warn() to raise an OSError instead of logging? (it is a neat kludge, but it should IMO be explained in the code.)
|
except for the missing comment in the unit test this is good to go IMO |
|
Added, thanks. Each probe keeps its The diff is additive, 104 lines and no deletions, so a parser raising something the probe does not recognise behaves exactly as it does on master and is merely visible now. That also inverts what the PR did before: the unforeseen exception no longer reaches the caller. I read your request that way because the risk worth covering is 443 tests pass, ruff and mypy clean. The test for an unrelated error now asserts the warning rather than the exception, and a second one asserts that a string which is simply not a database warns about nothing. Both fail without the handler. The branch was 81 commits behind, so it is rebased on master. |
|
@Poseidonas: can you add a comment in the unit test explaining how the "unforeseen" exception gets triggered? |
load_string() tries each supported format in turn and remembers why every one of them failed, so that it can report all five reasons together. Each attempt was wrapped in a bare `except Exception`, which also caught errors that say nothing about the format: cantools#814 reports an OSError raised by a logging handler while cantools emits a warning of its own, surfacing as `UnsupportedDatabaseFormatError: DBC: "[Errno 1] Incorrect function"` on a file that parses fine. Each probe now recognises what a parser raises when the content is not in its format. That set was established by feeding each parser every other format plus junk and an empty string, and recording what came out: cantools' own Error, textparser.ParseError and ElementTree.ParseError, plus ValueError and KeyError, which arxml, kcd and cdd use for a wrong root tag. The last two are wider than one would like, but they are what those parsers raise today, and narrowing them further would change exceptions callers can already see. Anything else keeps the behaviour it has today, reaching the combined UnsupportedDatabaseFormatError, and additionally emits a warning carrying the traceback and asking for a bug report. A parser raising something unforeseen is then visible instead of being reported as a bad format. Auto-detection is unaffected: loading all 131 database files under tests/ without an explicit format gives the same result for every one of them. Signed-off-by: George Vasiliades <1221374+Poseidonas@users.noreply.github.com>
5865d98 to
a22278b
Compare
|
Added. |
|
looks good now, merging. Thanks for the contribution! |
The dbc loader moved into a package and gained two checks for leftover VAL_ and SIG_VALTYPE_ entries which raise ValueError; they now raise ParseError like the rest. load_string() keeps the probe warning that came with cantools#827 rather than logging, so the tests for the three probe outcomes assert on the warning. Signed-off-by: George Vasiliades <1221374+Poseidonas@users.noreply.github.com>
Fixes #814.
load_string()probes each supported format in turn and keeps the failure from each one so that it can report all five reasons together if none of them works. Every attempt was wrapped in a bareexcept Exception, so an error that says nothing about the format was recorded as if it did.The reporter's case, reproduced unchanged on
c1dcd39:The file parses fine. The
OSErrorcomes from a logging handler that fails while cantools emits its own "Overwriting message ..." warning, and the traceback pointing at it is lost.After the change the same script raises the
OSErroritself, with the handler still in the traceback.Which exceptions actually mean "wrong format"
Rather than guess, I fed every parser each of the other formats plus junk and an empty string, and recorded what came out — 30 combinations:
ElementTree.ParseErrorcantools.errors.Errortextparser.ParseErrorValueErrorKeyErrorThe last two are wider than I would like. They come from three places that reject a wrong root tag with a plain builtin:
arxml/__init__.py:57—ValueError("No XML namespace specified or illegal root tag name ...")kcd.py:475—ValueError("Expected root element tag ...")cdd.py:244—KeyError("Could not find ECUDOC root element!")Making those raise
ParseErrorwould be tidier and would let the probe narrow to cantools' own errors plus the two parser ones. I left it alone because those exceptions are visible to anyone callingadd_arxml_string()and friends directly, and several tests assert on them — that seemed like a separate decision rather than something to fold into a bugfix. Happy to do it if you would prefer that shape.Verification
tests/with no explicit format, before and after. Same outcome for every file, including the 12 that already failed.UnsupportedDatabaseFormatError.masterwithUnsupportedDatabaseFormatErrorand passes with the change; the second guards the other direction, that a genuinely bad format is still reported as one.ruffandmypyclean. Suite goes 415 → 417 passing.test_cache_env_varfails on my machine both before and after — unrelated to this change.