Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace lit.error by raise LoadError #348

Merged
merged 5 commits into from
Jun 22, 2024
Merged

Conversation

tovrstra
Copy link
Member

@tovrstra tovrstra commented Jun 22, 2024

This is another step in #191. As a sideline in fixing that issue, the IOData error handling is improved in general, not just for errors and warnings when data is converted before writing it out.

The old code used a wrapper function lit.error to raise an exception when a file cannot be read for some reason. This function adds some info (filename and line number) to the error message. This was a bad idea for several reasons:

  • It does not allow raise LoadError(...) from exc calls.
  • It is less readable than a plain raise LoadError(....
  • Static code analysis does realize that lit.error(...) interrupts the control flow, leaving poor code undetected. (We had some of that in the gro and cp2klog formats, fixed in this PR.)

The logic that was in LineIterator.error has now been implemented in the LoadError exception, and has been generalized to work well for all use cases.

I will YOLO-merge this on Friday, June 28, unless reviewed earlier.

Summary by Sourcery

This pull request refactors error handling by replacing the custom lit.error function with direct raise LoadError calls. This change improves code readability, allows for better exception chaining, and enhances error reporting by including filename and line number information in the LoadError exception. Additionally, test cases have been updated to reflect these changes.

  • Enhancements:
    • Replaced the custom lit.error function with direct raise LoadError calls to improve readability and allow raise ... from syntax.
    • Generalized the LoadError exception to include filename and line number information, enhancing error reporting across various file formats.
  • Tests:
    • Updated test cases to check for LoadError exceptions instead of lit.error messages.

Copy link
Contributor

sourcery-ai bot commented Jun 22, 2024

Reviewer's Guide by Sourcery

This pull request replaces the lit.error method with raise LoadError across multiple files to improve error handling and readability. The LoadError class now includes enhanced error message handling, providing more context in error messages. Test cases have also been updated to reflect these changes.

File-Level Changes

Files Changes
iodata/formats/mwfn.py
iodata/formats/gromacs.py
iodata/formats/json.py
iodata/formats/molden.py
iodata/formats/wfx.py
iodata/formats/molekel.py
iodata/formats/fchk.py
iodata/formats/wfn.py
iodata/formats/charmm.py
iodata/formats/cp2klog.py
iodata/formats/fcidump.py
iodata/formats/gamess.py
iodata/formats/sdf.py
iodata/formats/gaussianinput.py
iodata/formats/mol2.py
iodata/formats/pdb.py
Replaced lit.error calls with raise LoadError in multiple locations and updated error messages to include more context.
iodata/test/test_wfx.py
iodata/test/test_gaussianinput.py
Updated test cases to check for LoadError instead of lit.error.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

deepsource-io bot commented Jun 22, 2024

Here's the code health analysis summary for commits be722a9..84e8f8f. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource Shell LogoShell✅ SuccessView Check ↗
DeepSource Python LogoPython✅ SuccessView Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @tovrstra - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 5 issues found
  • 🟢 Security: all looks good
  • 🟡 Testing: 4 issues found
  • 🟢 Complexity: all looks good
  • 🟡 Documentation: 1 issue found

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

iodata/utils.py Show resolved Hide resolved
iodata/formats/mwfn.py Show resolved Hide resolved
iodata/formats/gromacs.py Show resolved Hide resolved
iodata/formats/json.py Show resolved Hide resolved
iodata/formats/wfn.py Show resolved Hide resolved
iodata/test/test_wfx.py Show resolved Hide resolved
iodata/test/test_wfx.py Show resolved Hide resolved
iodata/test/test_wfx.py Show resolved Hide resolved
iodata/test/test_gaussianinput.py Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
Copy link
Member

@PaulWAyers PaulWAyers left a comment

Choose a reason for hiding this comment

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

Seems OK to me.

@tovrstra
Copy link
Member Author

Thanks for checking Paul!

@tovrstra tovrstra merged commit b5bccd9 into theochem:main Jun 22, 2024
12 checks passed
@tovrstra tovrstra deleted the drop-lit-error branch June 22, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants