Instagram / LibCST

A concrete syntax tree parser and serializer library for Python that preserves many aspects of Python's abstract syntax tree
https://libcst.readthedocs.io/
Other
1.57k stars 192 forks source link

Avoid raising bare Exception #1168

Open zaicruvoir1rominet opened 4 months ago

zaicruvoir1rominet commented 4 months ago

Summary

Before tackling issue #457, there are some bare Exceptions thrown from within the code.

Here is the logic for changes in Exceptions:

I'm all for changes if some Exceptions changes are not OK,
In particular, there are some ParserSyntaxError which may not be meaningful - with raw_line=0, raw_column=0 params

Test Plan

Keep the current test plan.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 25.49020% with 114 lines in your changes missing coverage. Please review.

Project coverage is 91.29%. Comparing base (8b97600) to head (eb3f2a6). Report is 9 commits behind head on main.

Files Patch % Lines
libcst/matchers/_matcher_base.py 9.52% 19 Missing :warning:
libcst/_parser/conversions/expression.py 18.18% 9 Missing :warning:
libcst/_parser/conversions/statement.py 18.18% 9 Missing :warning:
...bcst/codemod/commands/convert_format_to_fstring.py 20.00% 8 Missing :warning:
libcst/_parser/parso/python/tokenize.py 12.50% 7 Missing :warning:
libcst/_parser/conversions/params.py 14.28% 6 Missing :warning:
libcst/codegen/gen_matcher_classes.py 16.66% 5 Missing :warning:
libcst/helpers/_template.py 0.00% 5 Missing :warning:
libcst/_nodes/base.py 20.00% 4 Missing :warning:
libcst/_parser/grammar.py 0.00% 4 Missing :warning:
... and 19 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1168 +/- ## ========================================== + Coverage 91.26% 91.29% +0.03% ========================================== Files 261 262 +1 Lines 26877 26899 +22 ========================================== + Hits 24529 24558 +29 + Misses 2348 2341 -7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kiri11 commented 4 months ago

This seems like an improvement, thank you. It's really hard to catch a specific Exception, I have to rely on its message. I hope this gets merged.

zaicruvoir1rominet commented 3 months ago

Thanks for the support @kiri11 ! You also made me realize some users were perhaps using exception messages in order to manage errors, so I kept all original Exception messages in order to avoid breaking changes.

I hope everything is now ready (apart from that CSTLogicError file location & docstring)

kiri11 commented 3 months ago

You're welcome! (although I didn't do anything) Keeping the exception messages backwards-compatible is a great idea, thank you!