Skip to content

improve error handling consistency#525

Merged
wlav merged 6 commits intoFramework-R-D:mainfrom
wlav:normalize-errors
Apr 14, 2026
Merged

improve error handling consistency#525
wlav merged 6 commits intoFramework-R-D:mainfrom
wlav:normalize-errors

Conversation

@wlav
Copy link
Copy Markdown
Contributor

@wlav wlav commented Apr 13, 2026

Originally noticed by @greenc-FNAL, see here: #495 (comment), there are cases in the current code where a C++ exception is thrown where to code can still be reasonably be expected to return to Python for error handling. This PR removes two cases of throwing C++ exceptions in favor of setting Python exceptions and returning nullptr.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 13, 2026

Codecov Report

❌ Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
plugins/python/src/modulewrap.cpp 83.33% 2 Missing and 2 partials ⚠️
@@            Coverage Diff             @@
##             main     #525      +/-   ##
==========================================
+ Coverage   85.10%   85.49%   +0.38%     
==========================================
  Files         142      142              
  Lines        3645     3653       +8     
  Branches      626      629       +3     
==========================================
+ Hits         3102     3123      +21     
+ Misses        326      313      -13     
  Partials      217      217              
Flag Coverage Δ
unittests 85.49% <83.33%> (+0.38%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
plugins/python/src/modulewrap.cpp 80.35% <83.33%> (+2.90%) ⬆️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0539dee...844d126. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@wlav wlav requested a review from greenc-FNAL April 14, 2026 02:02
Add two tests in PHLEX_REGISTER_PROVIDERS that call s.provide() with
bad return-type annotations, covering the `if (!annotations_to_strings(...))`
return-nullptr path in sc_provide (modulewrap.cpp lines 1197-1198) that
was flagged by CodeCov on PR Framework-R-D#525:
- Union type (List[int]|Dict[int,int]) triggers the !norm path
- Surrogate string triggers the !ann (PyUnicode_AsUTF8 failure) path

Agent-Logs-Url: https://github.com/Framework-R-D/phlex/sessions/ed722823-d51b-4db7-868b-51cfafe6a548

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@greenc-FNAL greenc-FNAL left a comment

Choose a reason for hiding this comment

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

LGTM—I assume return "" vs return nullptr was a conscious choice?

I asked Copilot to try to improve the test coverage for the flagged lines missing coverage here, and it came up with 8c28888: feel free to cherry-pick, take inspiration from, or ignore as you prefer.

@wlav
Copy link
Copy Markdown
Contributor Author

wlav commented Apr 14, 2026

LGTM—I assume return "" vs return nullptr was a conscious choice?

Yes; nullptr was never on the table, but I was thinking of going for std::optional there, then was afraid of triggering more coverage issues, so figured that since "" (no annotation) is always an error anyway and since this is not a public function, might as well deal with it this way.

…e-codecov

test: cover sc_provide annotation error paths in pyprovide.py
@wlav wlav merged commit 0e24b56 into Framework-R-D:main Apr 14, 2026
37 checks passed
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.

3 participants