Skip to content

fix: helper function to strip mime params#1050

Open
Pranavjeet-Naidu wants to merge 2 commits intoopenzim:mainfrom
Pranavjeet-Naidu:fix/counterhandler_MIMEstripping
Open

fix: helper function to strip mime params#1050
Pranavjeet-Naidu wants to merge 2 commits intoopenzim:mainfrom
Pranavjeet-Naidu:fix/counterhandler_MIMEstripping

Conversation

@Pranavjeet-Naidu
Copy link
Copy Markdown

handles #1000
Basically a helper function that strips MIME-types params.
should fix the article count issue with the regex validation in zim-tools, this in libzim.

@kelson42 kelson42 requested a review from veloman-yunkan March 21, 2026 08:55
Signed-off-by: Pranavjeet-Naidu <pranavjeetnaidu@gmail.com>
@Pranavjeet-Naidu Pranavjeet-Naidu force-pushed the fix/counterhandler_MIMEstripping branch from 2200ab0 to b9ee28c Compare March 23, 2026 22:08
@Pranavjeet-Naidu
Copy link
Copy Markdown
Author

@kelson42 is the RTD fail because of the rebase or something else that I have to fix?

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 24, 2026

Codecov Report

❌ Patch coverage is 42.85714% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.23%. Comparing base (efc4e2a) to head (0dfc3ff).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/tools.cpp 60.00% 0 Missing and 4 partials ⚠️
src/writer/counterHandler.cpp 0.00% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1050      +/-   ##
==========================================
- Coverage   56.26%   56.23%   -0.03%     
==========================================
  Files         101      101              
  Lines        5014     5027      +13     
  Branches     2186     2194       +8     
==========================================
+ Hits         2821     2827       +6     
- Misses        737      738       +1     
- Partials     1456     1462       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a small internal helper to normalize MIME types by removing parameters (everything after ;), and uses it when generating the writer “Counter” metadata so downstream tooling (zim-tools) can reliably validate/count articles by base MIME type.

Changes:

  • Add zim::stripMimeParameters() to strip MIME parameters and trim whitespace before the first ;.
  • Apply MIME-parameter stripping in CounterHandler before incrementing the MIME counter.
  • Add unit tests covering common and edge-case MIME strings for the new helper.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/tooltesting.cpp Adds tests for stripMimeParameters() behavior across typical and edge inputs.
src/tools.h Declares the new private helper API.
src/tools.cpp Implements stripMimeParameters().
src/writer/counterHandler.h Adds <string> include for the std::string counter key type.
src/writer/counterHandler.cpp Normalizes MIME types (strip params) before counting, preventing parameter variants from inflating counts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Pranavjeet-Naidu <pranavjeetnaidu@gmail.com>
@Pranavjeet-Naidu
Copy link
Copy Markdown
Author

Pranavjeet-Naidu commented Mar 24, 2026

@kelson42 the RTD fail is fixed, and about the codecov fail, ill need to add an integration test for the uncovered lines:

auto cleanMimetype = zim::stripMimeParameters(mimetype);
if (cleanMimetype.empty()) {
  return;
}
m_mimetypeCounter[cleanMimetype] += 1;

It'll probably be added in src/writer/creator.cpp, but doesn't seem necessary and the partials also seem unnecessary, please confirm. If no more changes are needed, I'll squash again.

@kelson42
Copy link
Copy Markdown
Contributor

@Pranavjeet-Naidu Thank younfor your PR, but I guess you have linked the wrong issue in your PR description!?

@Pranavjeet-Naidu
Copy link
Copy Markdown
Author

@Pranavjeet-Naidu Thank younfor your PR, but I guess you have linked the wrong issue in your PR description!?

Actually nope. I linked this issue because of your reply in the end over there. You concluded saying we have to strip MIME type paras. Please let me know if there's a better issue to link instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants