libzim icon indicating copy to clipboard operation
libzim copied to clipboard

Introduced Formatter in libzim

Open ShaopengLin opened this issue 1 year ago • 7 comments

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

ShaopengLin avatar Mar 03 '24 08:03 ShaopengLin

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

codecov[bot] avatar Mar 04 '24 13:03 codecov[bot]

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 avatar Mar 04 '24 18:03 ShaopengLin

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 avatar Mar 04 '24 20:03 ShaopengLin

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 avatar Mar 05 '24 22:03 ShaopengLin

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

ShaopengLin avatar Mar 06 '24 23:03 ShaopengLin

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 avatar Mar 07 '24 17:03 ShaopengLin

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

ShaopengLin avatar Mar 10 '24 02:03 ShaopengLin

Should we ignore the codecov atm?

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

mgautierfr avatar Mar 20 '24 17:03 mgautierfr