Cantera / cantera

Chemical kinetics, thermodynamics, and transport tool suite
https://cantera.org
Other
623 stars 349 forks source link

Added descriptive error text to assert statements #1816

Closed cpilko closed 4 days ago

cpilko commented 1 week ago

Changes proposed in this pull request

Closes #

If applicable, provide an example illustrating new features this pull request is introducing

Checklist

bryanwweber commented 1 week ago

Hi! Thanks for making this change. I think this is definitely an improvement.

Since you're editing these lines anyways, ideally we would actually change these into full if statements, raising an appropriate ValueError. The reason is that interpreter options can be set for Python that disable assert statements at runtime, so these guards would no longer be effective.

Please also address the changes suggested by the linter: https://github.com/Cantera/cantera/actions/runs/11936944028/job/33273168168?pr=1816, in particular making sure not to introduce any extra spaces at the ends of lines. You can continue to push changes to this branch to update the pull request.

Thank you!

codecov[bot] commented 5 days ago

Codecov Report

Attention: Patch coverage is 20.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 73.25%. Comparing base (ad41b31) to head (27312a5). Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
interfaces/cython/cantera/composite.py 20.00% 4 Missing and 4 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1816 +/- ## ========================================== - Coverage 73.26% 73.25% -0.02% ========================================== Files 383 383 Lines 54621 54625 +4 Branches 9099 9103 +4 ========================================== - Hits 40017 40013 -4 - Misses 11601 11605 +4 - Partials 3003 3007 +4 ```

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


🚨 Try these New Features:

bryanwweber commented 5 days ago

Thank you! The test failures on macOS don't seem related to the changes here, but let's see what happens this time. I will merge this as soon as the tests look good!