Skip to content

[next-major] Remove metadata conversion from python-libzim #188

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

Open
benoit74 opened this issue Feb 8, 2024 · 6 comments · May be fixed by #190
Open

[next-major] Remove metadata conversion from python-libzim #188

benoit74 opened this issue Feb 8, 2024 · 6 comments · May be fixed by #190
Assignees
Labels
good first issue Good for newcomers

Comments

@benoit74
Copy link
Contributor

benoit74 commented Feb 8, 2024

When adding metadatas, the python-libzim is doing a conversion for some types.

This should be removed because:

  • the python-libzim is meant to be only a thin wrapper around the C libzim
  • not all types are handled which is confusing (for instance we missed the conversion of tags list to a string)

This would need a major release since it is a breaking change.

@rgaudin rgaudin added the good first issue Good for newcomers label Feb 8, 2024
@aryanA101a aryanA101a linked a pull request Feb 19, 2024 that will close this issue
@rgaudin rgaudin changed the title Remove metadata conversion from python-libzim [next-major] Remove metadata conversion from python-libzim Oct 15, 2024
@siddheshwar-9897
Copy link

sir is this issue still open

@siddheshwar-9897
Copy link


As per the issue, the current implementation in python-libzim automatically converts certain types of metadata (such as datetime.date and datetime.datetime objects) when they are added. The conversion includes:

Converting dates to a YYYY-MM-DD string and encoding in UTF-8.
Automatically encoding strings in UTF-8.
This leads to two main problems:

Inconsistent Type Handling: Not all types are handled consistently, such as lists (e.g., tags) not being converted to strings.
Unnecessary Conversion: The conversion is not required, as libzim itself is likely capable of handling the raw metadata types.
Suggested Solution:
I propose we remove the automatic conversion logic entirely, which would simplify the code and avoid the inconsistencies. Specifically:

Remove date conversion: Stop automatically converting datetime.date and datetime.datetime objects into strings.
Remove string encoding: Stop automatically encoding strings into UTF-8 bytes.
Let libzim handle types directly: Allow libzim to handle the metadata content as it expects, letting users handle any necessary conversions on their own.

**This will:

Make python-libzim a more straightforward wrapper that doesn’t enforce unnecessary transformations.
Avoid confusion or issues with inconsistent conversions (like missing list-to-string handling).
Give users more flexibility and control over their metadata, ensuring they're working with the exact data format they need.
Since this would be a breaking change (as it removes the automatic conversion behavior), I suggest we plan for a major release and update the documentation accordingly to inform users of the change.**

Let me know your thoughts or if you'd like to discuss further.

Best regards,
Siddheshwar Kadm

@benoit74
Copy link
Contributor Author

I'm sorry, but I won't read your comment. This is way too long/verbose and using way too much emphasis for something as straightforward as removing few lines of code.

Propose a PR, but please be sharp.

@rgaudin
Copy link
Member

rgaudin commented Feb 24, 2025

I believe we have a PR already that's ready and awaits that next major

@benoit74
Copy link
Contributor Author

benoit74 commented Feb 24, 2025

I believe we have a PR already that's ready and awaits that next major

Indeed ! @aryanA101a PR is ready

@benoit74 benoit74 self-assigned this Feb 24, 2025
@benoit74
Copy link
Contributor Author

Cannot assign aryanA101a so assigning myself so that it is clear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants