libzim
libzim copied to clipboard
Introduced Formatter in libzim
Hi! I am a GSOC student this year and its my first PR here.
Changes:
- Added Formatter Class in src/tools.h. Revised comments to fit our purpose and changed the code from openzim/zim-tools#158 to be more standard.
- Replaced majority of usages of (o)stringstream with Formatter with a few exceptions
- Removed
imports that are no longer necessary to improve compilation time.
Discussion needed:
- I am not sure if we want the class to be public facing? If no, then the createZimExample.cpp should not be changed. The user would be confused what Formatter is.
- Classes with their own operator<< defined with std::ostream (e.g. blob.h) and functions that uses more complex features of stringstreams (e.g. class CapturedStderr) in my opinion should not need changes.
- Making ourselves std::ostream goes against our goal to keep things in a stringstream.
- Having the same featrues of stringstreams will likely mean inheritance.
Fixes #432
Codecov Report
Attention: Patch coverage is 12.17391%
with 101 lines
in your changes are missing coverage. Please review.
Project coverage is 58.04%. Comparing base (
f624344
) to head (c234ff2
).
Additional details and impacted files
@@ Coverage Diff @@
## main #862 +/- ##
==========================================
+ Coverage 57.73% 58.04% +0.30%
==========================================
Files 100 101 +1
Lines 4619 4617 -2
Branches 1936 1921 -15
==========================================
+ Hits 2667 2680 +13
+ Misses 672 667 -5
+ Partials 1280 1270 -10
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I am a little against using it in test/archive. The CapturedStderr
Class is using rdbuf to read the stream buffer which our class is intended to hide. It makes the class too coupled. If we want to replace all uses of (o)stringstream with Formatter, we should be using inheritance instead. When they need specific features of stringstreams I think it is best they just use the original classes.
Also, I think we should place the class in include/zim/tools.h. After installation, error.h would not be able to use this class.
Resolved most of the comments and moved to zim/tools.h to ensure installation is correct. Likely need another patch depending on the discussion of std::endl vs '\n' and archive.cpp
Fixed the compilation issue with __ostream_type from different platforms. @mgautierfr Can you run the CI again?
I am new to codecov. I believe I should just add test cases in the test folder that hit those code changes right? This might take a while...
Should we ignore the codecov atm? So these changes were not part of the covered sections. If we see the sentry report from other PRs, the exact sections are shown as missed. I do not think I have enough knowledge on how to reproduce all these error edge cases. @mgautierfr @kelson42
Should we ignore the codecov atm?
Yes, the formater is used a lot to generate error message. Difficult to fully test.