Bump minor version and add new api to get if archive has alias. #847
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #847 +/- ##
=======================================
Coverage 57.59% 57.60%
=======================================
Files 99 99
Lines 4589 4590 +1
Branches 1920 1922 +2
=======================================
+ Hits 2643 2644 +1
Misses 677 677
Partials 1269 1269 ☔ View full report in Codecov by Sentry. |
src/fileimpl.cpp
Outdated
| @@ -599,6 +599,10 @@ class Grouping | |||
| return zimReader->size(); | |||
| } | |||
|
|
|||
| bool FileImpl::mayHaveAliasEntry() const { | |||
| return header.getMinorVersion() >= 2; | |||
There was a problem hiding this comment.
Does ZIM format have version 5.2? Then this check is incorrect.
Besides it is not future-proof and will need to be updated when the major version is bumped.
There was a problem hiding this comment.
BTW, the same problem is present with the detection of whether the ZIM archive uses the new namespace scheme:
Line 359 in abf7c11
There was a problem hiding this comment.
Does ZIM format have version 5.2?
No. Major version 6 is same as 5 but with extended cluster (blob bigger than for 4GB).
It has been use in parallel for a time then only 6.
Minor version was 0, then 1 for new namespace scheme (both with major 5 or 6) and now 2 (with 6 only, but it would be valid even with major 5)
include/zim/archive.h
Outdated
| * This method is just a hint. | ||
| * If true, its means that zim archive has been created with a implementation | ||
| * allowing alias creation. | ||
| * If false, it means that zim archive has been created before we | ||
| * formalize alias concept. But it would be still valid (from the spec) | ||
| * to have alias in the archive. | ||
| */ | ||
| bool mayHaveAliasEntry() const; |
There was a problem hiding this comment.
The potential benefits of having such a method are not clear. If the implementation supporting alias creation records (e.g. in metadata) the information about actual presence of aliases in a ZIM archive (and that is made part of the ZIM spec) then this method could be replaced with a tri-value (true, false, unknown) hasAliases() which is more meaningful.
There was a problem hiding this comment.
The potential benefits of having such a method are not clear.
Just a hint which can be used at reader level (for example) to change behaviour. I believe we will use this just to perform checks in a differentiated manner based of ZIM version.
If the implementation supporting alias creation records (e.g. in metadata) the information about actual presence of aliases in a ZIM archive (and that is made part of the ZIM spec) then this method could be replaced with a tri-value (
true,false,unknown)hasAliases()which is more meaningful.
There is no real quick way to know AFAIK that we have aliases and therefore don't think this is really doable to do that efficiently. The "may" versuin seems good enough.
There was a problem hiding this comment.
I want to add that if this new method just generate to much lack of consensus. On my side, this is OK to remove it (which means implicitly that for know we always consider a ZIM having - potentially - aliases).
There was a problem hiding this comment.
If the implementation supporting alias creation records (e.g. in metadata) the information about actual presence of aliases in a ZIM archive (and that is made part of the ZIM spec) then this method could be replaced with a tri-value (
true,false,unknown)hasAliases()which is more meaningful.There is no real quick way to know AFAIK that we have aliases and therefore don't think this is really doable to do that efficiently. The "may" versuin seems good enough.
With the new libzim aliases are created via a public API (zim::writer::Creator::addAlias()) and therefore Creator can keep track of whether any aliases have been added to the ZIM archive and save that information in some standardized way. Implementing it should be trivial. Defining the enhancement of the ZIM spec may be harder than implementing it.
There was a problem hiding this comment.
@veloman-yunkan Sorry I have merged a bit quickly... I agree with your last comment, but the core of the discussion as been already run at #833 (comment)?! I believe it is good like this. If it is proven we need more, we will do more.
mgautierfr
force-pushed
the
bump_minor_version
branch
from
e821e91
to
447b96d
Compare
|
I've removed the new API. So now the PR is just changing the minor version. |
Fix #844