-
Notifications
You must be signed in to change notification settings - Fork 0
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
new python bindings: SqliteReportBuilder.from_pyreport()
#36
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #36 +/- ##
==========================================
- Coverage 98.62% 98.53% -0.09%
==========================================
Files 21 22 +1
Lines 6751 6776 +25
==========================================
+ Hits 6658 6677 +19
- Misses 93 99 +6
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, though this brings up an interesting question:
should the output path/file be the full responsibility of the caller?
In the sense that the API does not have the new
fallback, and always requires the caller to provide a file handle?
related to that, I believe the rust tests are currently leaking temporary files, as it does not look like you are using the tempfile
crate thus far.
is there a reason to force the responsibility to one side or the other? why not allow the caller to supply a file if they want to manage it and fall back to a tempfile created by the library if they don't care?
i think all of the tests use tempfiles/tempdirs. python uses the |
Thats why I would suggest delegating that responsibility to the caller. That "resolves" that question, along with slimming down the API surface and the potential for surprises. |
cd2c344
to
6f2b3fa
Compare
6f2b3fa
to
72e3eac
Compare
@Swatinem i've removed |
fixes codecov/engineering-team#2315
commit titles describe the changes. the result is python bindings like:
immediate next steps:
there is some awkwardness to sort out due to python expecting to access
SqliteReportBuilder
functionality (db inserts) andSqliteReport
functionality (.totals()
,.merge()
, converting back to pyreport) through a single handle. this PR punts that issue down the line but i am thinking about it