Skip to content
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

xlsx export: remove redundant code #541

Merged
merged 10 commits into from
Mar 5, 2023

Conversation

matthewhegarty
Copy link
Contributor

@matthewhegarty matthewhegarty commented Mar 4, 2023

This PR removes what I believe is a redundant check from XLSX export dset_sheet(). It provides a minor simplification of the code.

If we check the source for openpyxl.cell.cell.Cell, the cell value setter calls _bind_value(). This method can raise a ValueError but not a TypeError. Therefore I have removed the checks for TypeError, and I assume that if this does occur somehow, then we would want this raised and not caught because this might be masking an error the user might be interested in.

I also removed catching TypeError when str(col) is called for the same reason.

I have added tests to increase the coverage.

@codecov
Copy link

codecov bot commented Mar 4, 2023

Codecov Report

Merging #541 (61ae056) into master (bff435d) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
+ Coverage   91.28%   91.39%   +0.11%     
==========================================
  Files          28       28              
  Lines        2719     2720       +1     
==========================================
+ Hits         2482     2486       +4     
+ Misses        237      234       -3     
Impacted Files Coverage Δ
src/tablib/formats/_xlsx.py 100.00% <100.00%> (+2.97%) ⬆️
tests/test_tablib.py 98.73% <100.00%> (+<0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

_xlsx = data.export('xlsx')
wb = load_workbook(filename=BytesIO(_xlsx))
# note the Cell is mocked so we don't get the 'real' value
self.assertEqual(None, wb.active['A1'].value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we could try with a value really producing a ValueError instead of this mocking/patching dance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - have used an 'array' value which openpyxl cannot handle. This is probably clearer but means we are also testing openpyxl code, instead of mocking as an external dependency. So if they ever change their implementation to handle arrays, this test will break.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, but anyway I tend to favor this simpler version. I think the former mocking solution was also sensible to implementation changes. @hugovk might arbitrate that choice 😄

Copy link
Member

Choose a reason for hiding this comment

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

Also prefer this simpler version, we'll just have to update the test if they change the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing guys - happy to go with the simpler approach. Over to you if you are happy to merge.

@claudep claudep merged commit 0857681 into jazzband:master Mar 5, 2023
@matthewhegarty matthewhegarty deleted the xls-tests-for-bad-col-values branch March 5, 2023 17:24
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