Lower minimum poppler-cpp version to 0.26.0 #13
Conversation
| if version() < (0, 46, 0): | ||
| assert date.astimezone(timezone.utc) == datetime( | ||
| 2020, 3, 25, 21, 19, 50, tzinfo=timezone.utc | ||
| ) |
There was a problem hiding this comment.
Strange... Do you think this is related to:
cpp:
* switched from detail::convert_date() to core's dateStringToTime()
in version 0.45?
There was a problem hiding this comment.
Are-you sure the wrong date is related with the poppler version? On my side, the date is the same with older poppler version.... Are you sure you computer has the right timezone?
There was a problem hiding this comment.
I'll take a closer look, I'm using inside an AWS SageMaker notebook, so it might not be set. However, maybe there is somewhere else the timezone is being used incorrectly.
| if version() < (0, 46, 0): | ||
| assert info == "Charles" |
There was a problem hiding this comment.
How to you explain this? Do you think it is related to
cpp: * pass len to GooString constructor in detail::ustring_to_unicode_GooString(). Bug #96426
in poppler 0.46?
There was a problem hiding this comment.
- test_page.py::test_text is failing. I get "Page " instead of "Page 1". The test should probably be updated.
- test_page.py::test_page_label is failing. I get "" instead of "1". Not sure why. Should we skip the test or mark it as xfail for this poppler version?
- test_document.py::test_get_property_with_locked_document and test_set_property_with_locked_document are failing because the properties are not available in this version. I think that in document.py, we could fall back to info_key and info_date methods for lower versions of poppler, instead of disabling the author, creator, creation_date, keywords and other properties.
| if version() < (0, 46, 0): | ||
| assert date.astimezone(timezone.utc) == datetime( | ||
| 2020, 3, 25, 21, 19, 50, tzinfo=timezone.utc | ||
| ) |
There was a problem hiding this comment.
Are-you sure the wrong date is related with the poppler version? On my side, the date is the same with older poppler version.... Are you sure you computer has the right timezone?
src/poppler/document.py
Outdated
| @property | ||
| @since(0, 46) | ||
| @ensure_unlocked | ||
| def author(self): | ||
| return str(self._document.get_author()) |
There was a problem hiding this comment.
Just to be sure you understood my comment, here is an example:
| @property | |
| @since(0, 46) | |
| @ensure_unlocked | |
| def author(self): | |
| return str(self._document.get_author()) | |
| @property | |
| @ensure_unlocked | |
| def author(self): | |
| try: | |
| value = self._document.get_author() | |
| except AttributeError: | |
| value = self._document.info_key('author') | |
| return str(value) |
And so on for other properties. This way, you should be able to re-enable some tests.
There was a problem hiding this comment.
Is there any reason not to always use .info_key all the time here?
There was a problem hiding this comment.
This is a good question. No, I don't think there is any reason, except that the function exists. We could definitively use info_key() and set_info_key() all the time.
|
I think you could add poppler-0.26.0 to the test matrix in python-poppler/.github/workflows/pythonpackage.yml . This way, the github runner would run the tests against this version as well. |
|
Hi @cbrunet, I've pushed the changes we've discussed. The tests were still failing for the dates and string truncation issues with poppler 0.26.0 on the Github workflow server, so I left the changes for conditional checks based on version in the tests. Let me know if anything else needs to be addressed. |
Amazon Linux ships with a super old version of
poppler-cpp. The following pull request makes changes to lower the minimumpoppler-cppversion to 0.26.0.