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

Implement handling of merged cells for .xlsx workbooks #226

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rvanheusden
Copy link

This PR adds handling for the mergeCells tag by parsing it and then editing the created Range before returning it. Perhaps there is a better way to go about this, but operating on the Range rather than the Vec of cells seemed like a better option.

@rvanheusden
Copy link
Author

It seems that my implementation fails the xlsx_richtext_namespaced test; I'll try to look into this soon and hopefully add a test for merged cells as well.

@LovingMelody
Copy link
Contributor

Attempted to figure out whats going wrong here, but this is beyond me. I'm going to leave my findings here in case it proves any help to you. It looks like your dimensions for merge is A1:G3 and H1:M3 resulting in the dimensions

Dimensions { start: (0,0), end: (2,6) }
Dimensions { start: (0,7), end: (2,12) }

but the range you are getting from sparse is [(0,0)..(0,6)]

@rvanheusden
Copy link
Author

Thanks for taking a look into this @Fuzen-py. I think that the issue here was my usage of subscripting for reading/writing Range cells. I updated the write method so that it uses the {get,set}_value methods instead. This way, when a merged cell extends past the normal bounds of the range, set_value will actually resize the range as needed.

This does not, however, resolve the test failure. I still need to add a proper test for merge cells, but I would like to suggest that the xlsx_richtext_namespaced test should probably not use merged cells. In my mind, unit tests should try to only test a single feature. It makes sense to me to remove the merged cells from this test's workbook so that its success/failure is not contingent on merge cells working properly. This will probably require input from @tafia though.

@LovingMelody
Copy link
Contributor

It doesn't appear to explicitly test the functionality of merged cells, its just part of the process of the workbook. I don't think that writing a specific test just for merged cells in addition to the current test is bad though.

@rvanheusden
Copy link
Author

Right, but that's what I'm actually suggesting could be an issue. The test implicitly relies on merged cells working properly after this PR, whereas ideally the test will fail/succeed regardless of whether or not the implementation of merged cells is correct.

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