Skip to content

bugfix: Fix an occasional compatibility issue when using Excel formulas #380

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
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sanshi42
Copy link

Problem
When loading an Excel file with complex formulas, an AttributeError is raised:

argtext = listsep.join(arg.text for arg in stack[-nargs:])
^^^^^^^^
AttributeError: 'int' object has no attribute 'text'

This happens because some invalid formula arguments are pushed to the stack as raw types (e.g., int) instead of wrapped Operand instances.

Solution
Ensure all arguments pushed onto the formula stack are properly wrapped in Operand objects, even when the formula is malformed. This prevents attribute access errors during parsing.

Copy link

codecov bot commented May 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.30%. Comparing base (0c4e80b) to head (99270dd).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #380      +/-   ##
==========================================
+ Coverage   56.55%   57.30%   +0.75%     
==========================================
  Files          24       24              
  Lines        5526     5528       +2     
==========================================
+ Hits         3125     3168      +43     
+ Misses       2401     2360      -41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cjw296
Copy link
Member

cjw296 commented May 30, 2025

What leads you to encounter this? Curious why you're still using .xls files?

@sanshi42
Copy link
Author

sanshi42 commented Jun 4, 2025

成员

Thanks for asking! Some of our internal systems still produce .xls file. To support a business requirement involving legacy .xls files, I used xlrd.open_workbook to load them. However, certain older files triggered unexpected errors. After investigating the source code, I identified the cause and made a targeted fix. The issue was resolved, so I’m submitting this patch accordingly.

@cjw296
Copy link
Member

cjw296 commented Jun 4, 2025

Okay, if you want this to land, you'll need to add suitable unit tests to cover the code you've added and that demonstrate the problem.

@sanshi42
Copy link
Author

sanshi42 commented Jun 5, 2025

Ok, I've added a unit test test_evaluate_name_formula_with_invalid_operand() that loads a minimized .xls file containing a malformed formula:
=SUM('#REF!'!#REF!:OFFSET('#REF!'!#REF!,0,MONTH('#REF!'!#REF!)-1)).
This previously caused an AttributeError, which is now resolved by the patch.

@cjw296
Copy link
Member

cjw296 commented Jun 5, 2025

What is it that's resulting in that malformed formula? Wouldn't it be better to fix that?

@sanshi42
Copy link
Author

sanshi42 commented Jun 5, 2025

What is it that's resulting in that malformed formula? Wouldn't it be better to fix that?

I’m not entirely sure how this particular malformed formula was created—modern Excel immediately prevents manual entry of such expressions. It may have originated from an older export or another tool outside our control. Since our goal is to parse user-provided workbooks without altering their original data, it would be more user-friendly to handle this scenario gracefully in xlrd rather than requiring users to clean up legacy files.

Copy link
Member

@cjw296 cjw296 left a comment

Choose a reason for hiding this comment

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

The testing is not sufficient.

What is the result of the change you've made in formula.py? How does this surface in the resulting xlrd objects?


class TestInvalidFormula(unittest.TestCase):
def test_evaluate_name_formula_with_invalid_operand(self):
xlrd.open_workbook(from_sample('invalid_formula.xls'))
Copy link
Member

Choose a reason for hiding this comment

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

This test needs to make assertions.
Please match the style of existing tests and find the best module to add them to.

xlrd.open_workbook(from_sample('invalid_formula.xls'))


if __name__ == '__main__':
Copy link
Member

Choose a reason for hiding this comment

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

Don't add block like this, run the tests in the existing way, the developer docs cover this.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I added the assertions and cleaned up the test style as suggested.
Let me know if there’s anything else — thanks for the review!

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.

2 participants