Skip to content

Avoid raising bare Exception#1168

Merged
zsol merged 15 commits intoInstagram:mainfrom
zaicruvoir1rominet:feature/avoid_raising_Exception
Jun 7, 2025
Merged

Avoid raising bare Exception#1168
zsol merged 15 commits intoInstagram:mainfrom
zaicruvoir1rominet:feature/avoid_raising_Exception

Conversation

@zaicruvoir1rominet
Copy link
Copy Markdown
Contributor

@zaicruvoir1rominet zaicruvoir1rominet commented Jul 4, 2024 •
edited
Loading

Summary

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

Here is the logic for changes in Exceptions:

  • use ValueError when issue caused by user input
  • use CSTValidationError & TypeError when issue occurs during the CST validation process
  • use ParserSyntaxError when issue occurs during the parsing process
  • use CSTLogicError to replace all Exception which contained "Logic error" in the cause message
  • remove Exception catch+re-raise when not needed

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.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 4, 2024
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 4, 2024 •
edited
Loading

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 ⚠️
libcst/_parser/conversions/expression.py 18.18% 9 Missing ⚠️
libcst/_parser/conversions/statement.py 18.18% 9 Missing ⚠️
...bcst/codemod/commands/convert_format_to_fstring.py 20.00% 8 Missing ⚠️
libcst/_parser/parso/python/tokenize.py 12.50% 7 Missing ⚠️
libcst/_parser/conversions/params.py 14.28% 6 Missing ⚠️
libcst/codegen/gen_matcher_classes.py 16.66% 5 Missing ⚠️
libcst/helpers/_template.py 0.00% 5 Missing ⚠️
libcst/_nodes/base.py 20.00% 4 Missing ⚠️
libcst/_parser/grammar.py 0.00% 4 Missing ⚠️
... and 19 more
Additional details and impacted files 
@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kiri11
Copy link
Copy Markdown
Contributor

kiri11 commented Jul 27, 2024 •
edited
Loading

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.

zsol
zsol suggested changes Jul 30, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@zsol zsol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I've got a few minor comments but overall looks great!

Comment thread libcst/_excep.py Outdated
Comment thread libcst/codemod/visitors/_add_imports.py Outdated
Comment thread libcst/codemod/visitors/_remove_imports.py
Comment thread libcst/codemod/visitors/_remove_imports.py Outdated
Comment thread libcst/metadata/type_inference_provider.py Outdated
@zaicruvoir1rominet
Copy link
Copy Markdown
Contributor Author

zaicruvoir1rominet commented Aug 12, 2024

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
Copy link
Copy Markdown
Contributor

kiri11 commented Aug 12, 2024

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

zaicruvoir1rominet and others added 6 commits April 10, 2025 23:35
Merge branch 'main' into feature/avoid_raising_Exception
1f7d51d
Update following merge
bcc87fb
* Move get_expected_str out of _exceptions.py, where it does not …
07f8df4 
…belong, to its own file in _parser/_parsing_check.py

* Group all exceptions in _exceptions.py
* Remove now empty file _excep.py (now redundant with _exceptions.py)
Linters pass
4b0afaa
@zaicruvoir1rominet
Merge branch 'main' into feature/avoid_raising_Exception
b2046e5 
# Conflicts:
#	libcst/codegen/gen_matcher_classes.py
@zaicruvoir1rominet
Merge & update
be89e25
@zaicruvoir1rominet zaicruvoir1rominet requested a review from zsol June 5, 2025 01:41
@zsol zsol merged commit ca1f81f into Instagram:main Jun 7, 2025
37 checks passed
jorenham pushed a commit to jorenham/LibCST that referenced this pull request Feb 3, 2026
@zaicruvoir1rominet @jorenham
Avoid raising bare Exception (Instagram#1168)
21d1f6e 
* Keep old exception messages (avoid breaking-changes for users relying on exception messages)

* Move ``get_expected_str`` out of _exceptions.py, where it does not belong, to its own file in _parser/_parsing_check.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@zsol zsol zsol approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zaicruvoir1rominet @kiri11 @zsol @facebook-github-bot