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 asserts by errors or warnings #365

Merged
merged 2 commits into from
Jul 7, 2024

Conversation

tovrstra
Copy link
Member

@tovrstra tovrstra commented Jul 6, 2024

An item on the list of cleanups in #204. This PR builds on the error and warning features added in the course of fixing #191.

The bad practice of using assert outside unit tests is removed. Such asserts are ignored by Python when running in optimized mode, meaning that possible errors when loading or dumping files go unnoticed. Each assert was checked and one of the following changes was made (roughly in order of decreasing prevalence):

  • Converted to raising a suitable error. (some moved to an earlier part of the code)
  • Converted to raising a warning.
  • Removed if the assertion was blocking something that was actually fine.

Other related changes:

  • Extended a few docstrings.
  • Improved some of the checks in the Molekel format (and removed a few bogus ones).
  • Moved tests for tools/harmonics.py to a separate file.

I will YOLO-merge this on Wednesday, July 10, unless reviewed earlier.

Summary by Sourcery

This pull request replaces assert statements with appropriate error or warning raises to ensure proper error handling in optimized mode. It also includes improvements to error messages and checks in the Molekel format, extends some docstrings, and moves tests for tools/harmonics.py to a separate file.

  • Bug Fixes:
    • Replaced assert statements with error or warning raises to ensure proper error handling in optimized mode.
  • Enhancements:
    • Improved error messages and checks in the Molekel format.
    • Extended docstrings for better clarity and documentation.
  • Tests:
    • Moved tests for tools/harmonics.py to a separate file tools/test_harmonics.py.

Copy link
Contributor

sourcery-ai bot commented Jul 6, 2024

Reviewer's Guide by Sourcery

This pull request replaces the use of assert statements with appropriate error or warning handling in various files. The changes ensure that potential issues are not ignored when Python is run in optimized mode. Additionally, some docstrings were extended, checks in the Molekel format were improved, and tests for tools/harmonics.py were moved to a separate file.

File-Level Changes

Files Changes
iodata/formats/extxyz.py
iodata/formats/molekel.py
iodata/formats/fchk.py
iodata/formats/cp2klog.py
iodata/formats/molden.py
iodata/formats/fcidump.py
iodata/formats/qchemlog.py
iodata/formats/wfx.py
iodata/formats/mwfn.py
iodata/formats/wfn.py
Replaced assert statements with appropriate error or warning handling to ensure potential issues are not ignored in optimized mode.
tools/harmonics.py
tools/test_harmonics.py
Moved tests for harmonics.py to a separate file and added new unit tests.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • 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 Jul 6, 2024

Here's the code health analysis summary for commits b08f401..37a67f1. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource Shell LogoShell✅ SuccessView Check ↗
DeepSource Python LogoPython❌ Failure
❗ 7 occurences introduced
🎯 44 occurences resolved
View 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: 17 issues found
  • 🟢 Security: all looks good
  • 🟡 Testing: 1 issue found
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

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/formats/extxyz.py Outdated Show resolved Hide resolved
iodata/formats/molekel.py Show resolved Hide resolved
iodata/formats/molekel.py Show resolved Hide resolved
iodata/formats/molekel.py Show resolved Hide resolved
iodata/formats/molekel.py Show resolved Hide resolved
iodata/formats/mwfn.py Show resolved Hide resolved
iodata/formats/mwfn.py Show resolved Hide resolved
iodata/formats/wfn.py Show resolved Hide resolved
iodata/test/test_prepare.py Show resolved Hide resolved
iodata/formats/fcidump.py Show resolved Hide resolved
@tovrstra
Copy link
Member Author

tovrstra commented Jul 6, 2024

The 7 deepsource issues already exists. This PR actually fixes 37 issues detected by deepsource and touched code related to those 7 other ones.

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.

I took a very fast look and it seems Ok to me.

@tovrstra
Copy link
Member Author

tovrstra commented Jul 7, 2024

Thanks for checking Paul! Merging...

@tovrstra tovrstra merged commit 22475e1 into theochem:main Jul 7, 2024
11 of 12 checks passed
@tovrstra tovrstra deleted the rewrite-assert branch July 7, 2024 05:21
@tovrstra tovrstra mentioned this pull request Jul 7, 2024
22 tasks
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.

2 participants