Fix confusing ReadError traceback #262
Conversation
ssbarnea
force-pushed
the
master
branch
2 times, most recently
from
b161159
to
ed3946b
Compare
ssbarnea
force-pushed
the
master
branch
2 times, most recently
from
3117451
to
85bae6e
Compare
|
@adrienverge Does this look ok? |
|
Wow, reviving a PR after one day is a bit too much. I have concerns about this commit, I'll take the time to think about it. Please be patient. |
There was a problem hiding this comment.
Thanks @ssbarnea!
My initial preferred solution was to re-raise an error that would contain both the problematic file name and the precise exception encountered:
try:
syntax_error = get_syntax_error(buffer)
except yaml.reader.ReaderError as e:
raise Exception('Read error on file ' + filepath) from eBut your proposal is more user-friendly, as it reports the error without stopping.
Can you add tests and fix the small problem below? Thanks
tests/test_syntax_errors.py
Outdated
|
|
||
| def test_non_printable_characters(self): | ||
| self.check('output: \x1b\n', | ||
| None, problem=(1, 9)) |
There was a problem hiding this comment.
Please add more tests. At the least I can think of:
- invalid character in the first byte,
- invalid character in the last byte,
- invalid character not on line 1,
- multiple invalid characters,
- test with other rules enabled.
There was a problem hiding this comment.
Sorry for the long delay, somehow this patch got lost and only rediscovered it yesterday. I added the tests.
Still, I am unsure if the coverage failure is a bug in coverage because these new tests would cover that piece of code. Also there are no instructions about running coverage locally for the project so I hope you may be able to provide few hints here.
ssbarnea
changed the title
Avoids confusing tracebacks when ReadErrors exceptions are raised from the yaml reader. Now we do print the filename that failed to load and stop processing in order to avoid additional exceptions. Also adds tests to prevent future regressions. Fixes: adrienverge#261
adrienverge
changed the title
Co-authored-by: Adrien Vergé <[email protected]>
Avoids confusing tracebacks when ReadErrors exceptions are raised
from the yaml reader.
Now we do print the filename that failed to load and stop processing
in order to avoid additional exceptions.
Also adds tests to prevent future regressions.
Example output after this change:
Fixes: #261