ENH: Enhance XMP metadata handling with creation and setter methods #3410
Conversation
Arya-A-Nair
force-pushed
the
feature/xmp-create-method
branch
from
9183c12 to
7129e69
Compare
- Fix missing Dict and List imports in typing - All XMP setter methods are now working correctly - All tests pass except one unrelated remote file test - XmpInformation.create() and all setter methods fully functional - Code follows existing codebase patterns and style
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3410 +/- ##
==========================================
+ Coverage 96.97% 97.04% +0.06%
==========================================
Files 54 54
Lines 9337 9567 +230
Branches 1711 1739 +28
==========================================
+ Hits 9055 9284 +229
Misses 168 168
- Partials 114 115 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add tests for ownerDocument None error conditions (lines 459-465, 484, 506, 535, 564, 597) - Add tests for description element creation paths - Add tests for attribute handling and edge cases - Improve test coverage from 95% to 97% - All XMP functionality thoroughly tested with error conditions and edge cases
|
Thanks for the PR. Besides the coverage: Could we please have proper properties instead of a read-only property and a setter? This is not really pythonic. |
|
@stefan6419846 yeah sure, I understood my mistake. I'll refactor it and ask for review, once done |
Arya-A-Nair
force-pushed
the
feature/xmp-create-method
branch
from
d66f82b to
1abdf04
Compare
|
@stefan6419846 you can take a look at it now |
|
Sorry for the delays - I did not yet have the time to properly review these larger changes again. I hope that I manage to look into this in the next days. |
|
Hey @stefan6419846 can i contribute to some other issue till that time? |
|
You can of course look into another issue if you start the corresponding changes from the main code again. |
| del self.cache[namespace][name] | ||
| desc = self._get_or_create_description() | ||
|
|
||
| existing_elements = list(desc.getElementsByTagNameNS(namespace, name)) |
There was a problem hiding this comment.
Why do we need the list conversion and a separate variable?
There was a problem hiding this comment.
I didnt quite understand this, Can you clarify on it?
There was a problem hiding this comment.
Will there be any issues if we use the following direct code?
for elem in desc.getElementsByTagNameNS(namespace, name):
desc.removeChild(elem)There was a problem hiding this comment.
Yes, there would be issues with the direct approach. getElementsByTagNameNS() returns a live NodeList that automatically updates when the DOM changes. If we iterate directly over it while calling removeChild(), the NodeList shrinks and re-indexes during iteration, causing elements to be skipped. The list() conversion creates a static snapshot of the elements to remove, avoiding this iteration-while-modifying problem.
There was a problem hiding this comment.
If you think we are fine with this. Then I will go ahead and change it
…edicated methods for improved clarity and maintainability
…pInformation class
… improved maintainability
…te for consistency and clarity; update assertions in the test for improved accuracy
…treamline cache management for metadata values
…ith PDF 2.0 XMP recommendations and improve XML serialization
…MP values while preserving existing data
…erify incremental updates of XMP values without overwriting
… NAMESPACE_PREFIX_MAP to _NAMESPACE_PREFIX_MAP and MINIMAL_XMP to _MINIMAL_XMP
…ate documentation to reflect new approach for incrementally updating XMP metadata fields directly using standard data structures
…pdf into feature/xmp-create-method
|
Hey @stefan6419846 can you take a look at this PR? |
|
I am currently on tour and will not be able to do look into larger PRs until September. |
Closes #3394