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

Fix iterByTitle. #518

Closed
wants to merge 2 commits into from
Closed

Fix iterByTitle. #518

wants to merge 2 commits into from

Conversation

mgautierfr
Copy link
Collaborator

If we have a specific title index, we must iterate on it.
We must not iterate on a (wrong) subset of the entries.

Related to #514
(This doesn't try to fix #514)

@codecov
Copy link

codecov bot commented Mar 10, 2021

Codecov Report

Merging #518 (685888d) into master (d4801b6) will decrease coverage by 0.00%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #518      +/-   ##
==========================================
- Coverage   76.36%   76.35%   -0.01%     
==========================================
  Files          89       89              
  Lines        3655     3659       +4     
  Branches     1640     1661      +21     
==========================================
+ Hits         2791     2794       +3     
- Misses        863      864       +1     
  Partials        1        1              
Impacted Files Coverage Δ
src/archive.cpp 56.35% <66.66%> (-0.08%) ⬇️
src/fileimpl.cpp 84.09% <100.00%> (+0.04%) ⬆️
src/fileimpl.h 90.90% <100.00%> (+0.90%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4801b6...685888d. Read the comment docs.

src/fileimpl.h Outdated
@@ -89,6 +90,7 @@ namespace zim
const Fileheader& getFileheader() const { return header; }
zsize_t getFilesize() const;
bool hasNewNamespaceScheme() const { return m_newNamespaceScheme; }
bool hasFullTitleIndex() const { return m_hasFullTitleIndex; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't it make sense to signal the presence of a front article index instead? As far as I understand the full title index should be always present in a ZIM archive, but it is "overriden" by the front article index if the latter is available.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, as it is a internal method I haven't think a lot on the api (better to discuss in #514).
But your remark is right. And is is more coherent with hasNewNamespaceScheme which return true if we have something "new".

@kelson42
Copy link
Contributor

@mgautierfr @veloman-yunkan I don’t understand what is the status of this PR

@mgautierfr
Copy link
Collaborator Author

I had to change the PR following the @veloman-yunkan remarks.

This is done now.

If we have a specific title index, we must iterate on it.
We must not iterate on a (wrong) subset of the entries.

Related to #514
Rename hasFullTitleIndex to hasFrontArticleIndex.
@kelson42
Copy link
Contributor

kelson42 commented Apr 1, 2021

@mgautierfr Is this PR enough tested, codecov complains it does not meet our standarts.

@mgautierfr
Copy link
Collaborator Author

We are missing a test zim file with the new format.
If @veloman-yunkan agree, I would prefer to merge this PR now and think about how to have this test zim file elsewhere
(in https://github.com/openzim/libzim/issues/469 or in a specific issue)

@veloman-yunkan
Copy link
Collaborator

We are missing a test zim file with the new format.
If @veloman-yunkan agree, I would prefer to merge this PR now and think about how to have this test zim file elsewhere
(in #469 or in a specific issue)

I don't object, though I think that changes must be tested as soon as they are introduced.

@kelson42
Copy link
Contributor

kelson42 commented Apr 8, 2021

We are missing a test zim file with the new format.
If @veloman-yunkan agree, I would prefer to merge this PR now and think about how to have this test zim file elsewhere
(in #469 or in a specific issue)

I don't object, though I think that changes must be tested as soon as they are introduced.

I agree, should be automated tested before merging this PR.

@mgautierfr
Copy link
Collaborator Author

superseded by #535

@mgautierfr mgautierfr closed this Apr 23, 2021
@mgautierfr mgautierfr deleted the iter_by_title branch May 12, 2021 13:19
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.

Fix semantics about number of entry/article/front article/...(
3 participants