feat: Adding settings, utils, write to S3, and writer close #67
Conversation
malteos
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
==========================================
+ Coverage 95.78% 97.05% +1.27%
==========================================
Files 7 9 +2
Lines 877 918 +41
==========================================
+ Hits 840 891 +51
+ Misses 37 27 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Given that the test coverage check fails on this one, what about adjusting the coverage check to a fixed value instead of the last coverage value? |
|
While the coverage check failed, I don't think it will cause a problem in the next CI run? It's just a slightly easier hurdle next time. |
I added additional unit tests which should increase the coverage above the threshold. However, codecov seemed to stopped working after moving the repo to the CC org. I already requested access to the org. I guess you just need to approved codecov again @wumpus. |
|
The Codecov checks pass now. |
| LOGGER.warning('revisit record being resolved for url %s %s', url, timestamp) | ||
| writer.write_record(record) | ||
|
|
||
| writer.close() |
There was a problem hiding this comment.
knee-jerk reaction: can this be a with ...: context manager sorta deal?
|
|
||
| # remember: keep requires synchronized with requirements.txt | ||
| requires = ['requests', 'warcio'] | ||
| requires = ['requests', 'warcio', 'fsspec[s3]', 'boto3'] |
There was a problem hiding this comment.
boto3 appears to be missing from requirements.txt
| LOGGER.warning('surprised that status code is now=%d orig=%s %s %s', | ||
| status_code, capture['status'], url, timestamp) | ||
| LOGGER.warning( | ||
| 'surprised that status code is now=%d orig=%s %s %s', status_code, capture['status'], url, timestamp |
There was a problem hiding this comment.
'surprised that' is unusual phrasing. I guess you've just inherited older code here but it might be worth improving these messages. who is surprised? should I, the reader of the logs, be surprised? is it something I should be concerned about? is it my responsibility to fix?
| return cdx_toolkit.__version__ | ||
|
|
||
|
|
||
| def setup(cmd): |
There was a problem hiding this comment.
can we give this function a more descriptive name? what's it setting up exactly? is there a suitable type hint for 'cmd' (is it from argparse?)
damian0815
left a comment
damian0815
left a comment
There was a problem hiding this comment.
'request changes' I think they only blocker issue is the boto3 require
This PR integrates a couple of general changes from the EOT PR (#54):
settings.pyutils.pywriter.close()statement is added to CLI and example.