openzim / libzim

Reference implementation of the ZIM specification
https://download.openzim.org/release/libzim/
GNU General Public License v2.0
166 stars 49 forks source link

Introduced Formatter in libzim #862

Closed ShaopengLin closed 6 months ago

ShaopengLin commented 7 months ago

Hi! I am a GSOC student this year and its my first PR here.

Changes:

Discussion needed:

Fixes #432

codecov[bot] commented 7 months ago

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).

Files Patch % Lines
src/file_reader.cpp 0.00% 36 Missing and 1 partial :warning:
src/writer/creator.cpp 9.09% 4 Missing and 6 partials :warning:
src/narrowdown.h 0.00% 4 Missing and 5 partials :warning:
src/writer/cluster.cpp 0.00% 3 Missing and 5 partials :warning:
src/debug.h 0.00% 6 Missing :warning:
src/entry.cpp 28.57% 2 Missing and 3 partials :warning:
src/file_compound.cpp 0.00% 5 Missing :warning:
src/writer/counterHandler.cpp 0.00% 0 Missing and 4 partials :warning:
include/zim/error.h 0.00% 0 Missing and 3 partials :warning:
src/version.cpp 0.00% 3 Missing :warning:
... and 7 more
Additional details and impacted files ```diff @@ 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.

ShaopengLin commented 7 months ago

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.

ShaopengLin commented 7 months ago

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.

ShaopengLin commented 7 months ago

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

ShaopengLin commented 7 months ago

Fixed the compilation issue with __ostream_type from different platforms. @mgautierfr Can you run the CI again?

ShaopengLin commented 7 months ago

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...

ShaopengLin commented 7 months ago

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

mgautierfr commented 6 months ago

Should we ignore the codecov atm?

Yes, the formater is used a lot to generate error message. Difficult to fully test.