Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respecting the --ns option in zimdump show #316

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Conversation

veloman-yunkan
Copy link
Collaborator

Fixes #315

@codecov
Copy link

codecov bot commented Aug 14, 2022

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 27.84%. Comparing base (c673e5f) to head (82412c5).

Files Patch % Lines
src/zimdump.cpp 0.00% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #316      +/-   ##
==========================================
- Coverage   27.93%   27.84%   -0.09%     
==========================================
  Files          26       26              
  Lines        2585     2593       +8     
  Branches     1376     1381       +5     
==========================================
  Hits          722      722              
- Misses       1376     1384       +8     
  Partials      487      487              

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

@mgautierfr
Copy link
Collaborator

This PR add the ns to the path if --ns is provided.
It works and technically fix the behavior first explained by @kelson42 but it doesn't fix the global interface problem (and the solution proposed in #315 (comment))

We can merge this PR anyway but not sure we should really close #315

@kelson42
Copy link
Contributor

kelson42 commented Nov 25, 2023

@mgautierfr We need to know what is in the ZIM file. For the moment we can't properly because the "removal" of namespaces has broken zimdump. At this stage we are mostly interested to get the feature back before thinking of doing it better.

From a general POV, Libzim7 has introduced the idea that end-user visible content (so not the metadata, ...) would be "all" in the same namespace C. This, mostly to simplify scrapers. That does not mean that we hide things so far that even an introspection tool like zimdump is not able easily to retrieve what is a ZIM file or rely on the concept of a namespace. Namespace is a documented part of the ZIM specification.

If I agree to abstract/hide this concept for end-user, I think I can pretty much say: I disagree to hide it from the ZIM tools.

Copy link
Contributor

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

@veloman-yunkan Late, but this PR has not been abandoned!

I have tested:

$ zimdump show --ns=M --url=Name gutenberg_fa_all_2023-08.zim 
Entry not found

I don't know if I do something wrong but to me, it seems to fail (at least with latest libzim).

In addition, I have remarked that basically all the --ns options are broken. I would really like to have them working like announced and like they were. Today, I have basically lost more than one hour because - with zimdump - I was not even able to know if a ZIM file has a fulltext index or not! This is problematic.

@veloman-yunkan
Copy link
Collaborator Author

@kelson42 That's because of openzim/libzim#843

@veloman-yunkan
Copy link
Collaborator Author

In addition, I have remarked that basically all the --ns options are broken. I would really like to have them working like announced and like they were. Today, I have basically lost more than one hour because - with zimdump - I was not even able to know if a ZIM file has a fulltext index or not! This is problematic.

It is possible to fix zimdump without/before addressing openzim/libzim#843, but should we do that?

@mgautierfr
Copy link
Collaborator

It is possible to fix zimdump without/before addressing openzim/libzim#843, but should we do that?

Is the fix for openzim/libzim#843 would invalidate the fix of zimdump ? If no, we should fix zimdump without waiting for libzim IMO.

@veloman-yunkan
Copy link
Collaborator Author

It is possible to fix zimdump without/before addressing openzim/libzim#843, but should we do that?

Is the fix for openzim/libzim#843 would invalidate the fix of zimdump ? If no, we should fix zimdump without waiting for libzim IMO.

I was under the wrong impression (again mislead by the docstring) that zim::Archive::findByPath() respects the namespace and can be used instead of zim::Archive::getEntryByPath(). Or maybe I hoped to use findx(), having overlooked that it is a member function of FileImpl rather than Archive. In any case, that was a false call. :(

@kelson42
Copy link
Contributor

@veloman-yunkan I'm slightly lost about the status of this PR, but please let me know when this is ready to new functional review.

@veloman-yunkan veloman-yunkan force-pushed the zimdump_show_ns branch 2 times, most recently from 5d0ff3d to a2c9fa5 Compare March 3, 2024 14:26
@veloman-yunkan
Copy link
Collaborator Author

Now fixed taking advantage of zim::Archive::getEntryByPathWithNamespace() introduced in openzim/libzim#843

@kelson42
Copy link
Contributor

kelson42 commented Mar 6, 2024

I will check from a user perspective.

@kelson42 kelson42 merged commit 1f6a378 into main Mar 6, 2024
10 of 12 checks passed
@kelson42 kelson42 deleted the zimdump_show_ns branch March 6, 2024 19:25
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.

zimdump show weird behaviour
3 participants