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

Fix CONCATENATE for cell references #1266

Merged
merged 3 commits into from Aug 11, 2019

Conversation

@igitur
Copy link
Member

commented Aug 5, 2019

CONCATENATE may accept cell range references as parameters, but its implementation is tricky and we can't support the full scope that Excel supports. See added code comments for clarification. This improves it though.

Fixes #1264

@igitur igitur requested a review from Pankraty Aug 5, 2019

@igitur igitur added the bug label Aug 5, 2019

@igitur igitur added this to the v0.95 milestone Aug 5, 2019

var columnCount = cellRangeReference.Range.RangeAddress.LastAddress.ColumnNumber - cellRangeReference.Range.RangeAddress.FirstAddress.ColumnNumber + 1;

if (columnCount > 1)
throw new CellValueException("This function does not accept cell ranges as parameters.");

This comment has been minimized.

Copy link
@Pankraty

This comment has been minimized.

Copy link
@igitur

igitur Aug 5, 2019

Author Member

Oh, so the transpose is also valid.

In the screenshot you pasted, only E3 and F3 would currently be possble. So what should we do? Either return wrong values or throw an exception?

This comment has been minimized.

Copy link
@Pankraty

Pankraty Aug 5, 2019

Member

Maybe it is time to implement context-dependent functions? Like ROW(), COLUMN(), etc.

This comment has been minimized.

Copy link
@igitur

igitur Aug 5, 2019

Author Member

Yes, we should. But for now we need to fix this regression. I think we just return 1 text or 5 text for each value in the column/row.

This comment has been minimized.

Copy link
@Pankraty

Pankraty Aug 5, 2019

Member

I am not convinced that this is a regression as this function never worked with range references as it should. I would prefer the function screaming out loud that it is not fully implemented (=throw an exception) rather than silently returning an incorrect result that might be extremely hard to trace should it cause the error far down the stream.

This comment has been minimized.

Copy link
@igitur

igitur Aug 5, 2019

Author Member

The above use-case isn't a regression, but the CONCATENATE(A1, B1) case is a regression.

Yes, I now agree with you. Let's throw an exception. I doubt many people use that edge case anyway.

@igitur igitur force-pushed the igitur:issue1264-concatenate-regression branch from ef66263 to 064b1e4 Aug 7, 2019

@igitur

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

Added some IXLRangeAddress helper functions too.

{
var ws = new XLWorkbook().AddWorksheet() as XLWorksheet;

var range = ws.Range("B3:E5");

This comment has been minimized.

Copy link
@Pankraty

Pankraty Aug 7, 2019

Member

Try also B5:E3 and E5:B3. The results must be the same (now they are not). I suggest using Normalize() in the implementation.

@igitur igitur force-pushed the igitur:issue1264-concatenate-regression branch from 064b1e4 to b0a7b02 Aug 7, 2019

@igitur

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2019

@Pankraty Happy to merge?

@igitur igitur merged commit 019e79d into ClosedXML:develop Aug 11, 2019

2 checks passed

WIP Ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.