Skip to content

chore: address stale TODO comments across codebase#3108

Merged
ceberam merged 8 commits intodocling-project:mainfrom
M-Hassan-Raza:fix/address-todo-comments
Mar 14, 2026
Merged

chore: address stale TODO comments across codebase#3108
ceberam merged 8 commits intodocling-project:mainfrom
M-Hassan-Raza:fix/address-todo-comments

Conversation

@M-Hassan-Raza
Copy link
Contributor

Cleaned up 4 TODO/FIXME comments:

  • document_converter.py: log a warning when conversion fails silently (the TODO literally asked for this)
  • msword_backend.py: removed two stale TODO: prefixes — the code below them already does what they ask for
  • base_model.py: replaced manual bbox expansion with BoundingBox.expand_by_scale() from docling-core
  • verify_utils.py: added bbox comparison with tolerance in test verification

All e2e tests pass.

Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

DCO Check Passed

Thanks @M-Hassan-Raza, all your commits are properly signed off. 🎉

@mergify
Copy link

mergify bot commented Mar 11, 2026

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Enforce conventional commit

Wonderful, this rule succeeded.

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?(!)?:

Signed-off-by: Peter W. J. Staar <91719829+PeterStaar-IBM@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Mar 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Member

@ceberam ceberam left a comment

Choose a reason for hiding this comment

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

Thanks @M-Hassan-Raza . Could you please have a look at the failing tests?

- Extract duplicated "set marker + add list item" logic in
  _add_list_item() into _add_list_item_with_marker() helper method
- Add tests for defensive "no pipeline" code path in _execute_pipeline()

Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
@M-Hassan-Raza M-Hassan-Raza requested a review from ceberam March 12, 2026 14:55
@M-Hassan-Raza
Copy link
Contributor Author

@ceberam How about now, looking good enough?

Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
@M-Hassan-Raza
Copy link
Contributor Author

hmmmm some tolerance issues, seems to me that I enforced the actual ground truth of 2 decimal places but it broke existing checks, they seem to be needing an update as well. I'll take a look

Fuzzy tests (OCR, image) have inherent coordinate variability, so bbox
checks are skipped. For non-fuzzy tests, increase tolerance from 0.01
to 0.5 to account for cross-platform numeric variance.

Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
Signed-off-by: Hassan Raza <raihassanraza10@gmail.com>
@M-Hassan-Raza
Copy link
Contributor Author

Thank god all the tests passed 😅 Took a while to locally run tests beforehand

@PeterStaar-IBM
Copy link
Member

@ceberam let's approve this and merge

Copy link
Member

@ceberam ceberam left a comment

Choose a reason for hiding this comment

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

Great 🚀

@ceberam ceberam merged commit 6238aa3 into docling-project:main Mar 14, 2026
25 checks passed
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.

3 participants