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

Feature/#1010 text join func #1015

Merged
merged 3 commits into from Jan 10, 2019

Conversation

Projects
None yet
3 participants
@James-Whitfield
Copy link
Contributor

James-Whitfield commented Oct 5, 2018

What's this PR do?

This PR implements the excel function TEXTJOIN which joins strings together with a delimiter.
the docs for this can be viewed here.

Where should the reviewer start?

They can start by running the new unit test added to the project.

How should this be manually tested?

This can be manually tested by running the unit test or creating a formula which uses TEXTJOIN.

Any background context you want to provide?

This function was not supported. So I raised it as a issue #1010 and it was suggested that I have a go at implementing it. Here is my attempt.

Screenshots (if appropriate)

Questions:

  • Is there a blog post?
    No

  • Does the knowledge base need an update?
    Might need to add it to the list of supported functions

  • Does this add new (C#) dependencies?
    No

  • C# Code Review: @csreviewer

  • Test Automation Review: @csreviewer

@igitur
Copy link
Member

igitur left a comment

This PR should contain only 1 commit, so please squash and rebase before pushing.

Show resolved Hide resolved ClosedXML/Excel/CalcEngine/Functions/Text.cs Outdated
Show resolved Hide resolved ClosedXML/Excel/CalcEngine/Functions/Text.cs Outdated
Show resolved Hide resolved ClosedXML/Excel/CalcEngine/Functions/Text.cs Outdated

@James-Whitfield James-Whitfield force-pushed the James-Whitfield:feature/#1010-text-join-func branch from 52f0095 to 92559d5 Oct 8, 2018

@Pankraty Pankraty force-pushed the James-Whitfield:feature/#1010-text-join-func branch from e8f89bc to d791d79 Oct 14, 2018

@Pankraty Pankraty force-pushed the James-Whitfield:feature/#1010-text-join-func branch from d791d79 to 75f5dc0 Oct 15, 2018

@igitur

This comment has been minimized.

Copy link
Member

igitur commented Oct 15, 2018

Thanks for the work on this, @Pankraty .

I think this PR should depend on #1035 . Once #1035 has been merged, I think the TextJoin function can be improved. I'll make notes in the review.

@Pankraty

This comment has been minimized.

Copy link
Member

Pankraty commented Oct 15, 2018

I agree

@igitur

This comment has been minimized.

Copy link
Member

igitur commented Oct 15, 2018

Here is my alternative version of TextJoin(), which depends on #1035

private static object TextJoin(List<Expression> p)
{
    var values = new List<string>();
    var delimiter = (string)p[0];
    var ignoreEmptyStrings = (bool)p[1];

    foreach (var param in p.Skip(2))
    {
        if (param is XObjectExpression tableArray)
        {
            if (!(tableArray.Value is CellRangeReference rangeReference))
                throw new NoValueAvailableException("tableArray has to be a range");

            var range = rangeReference.Range;
            IEnumerable<string> cellValues;
            if (ignoreEmptyStrings)
                cellValues = range.CellsUsed()
                    .Select(c => c.GetString())
                    .Where(s => !string.IsNullOrEmpty(s));
            else
                cellValues = range.Cells().Select(c => c.GetString());

            values.AddRange(cellValues);
        }
        else
        {
            values.Add((string)param);
        }
    }

    var retVal = string.Join(delimiter, values);

    if (retVal.Length > 32767)
        throw new CellValueException();

    return retVal;
}
Show resolved Hide resolved ClosedXML/Excel/CalcEngine/Functions/Text.cs Outdated
Show resolved Hide resolved ClosedXML/Excel/CalcEngine/Functions/Text.cs Outdated
Show resolved Hide resolved ClosedXML_Tests/Excel/CalcEngine/TextTests.cs Outdated
Show resolved Hide resolved ClosedXML/Excel/CalcEngine/Functions/Text.cs Outdated
Show resolved Hide resolved ClosedXML/Excel/CalcEngine/Functions/Text.cs Outdated
@Pankraty

This comment has been minimized.

Copy link
Member

Pankraty commented Oct 15, 2018

Here is my alternative version of TextJoin(), which depends on #1035

Looks much nicer! But pay attention that range.Cells().Select(c => c.GetString()); is quite insufficient for column-wide ranges (e.g. A:B).

@igitur

This comment has been minimized.

Copy link
Member

igitur commented Oct 15, 2018

Looks much nicer! But pay attention that range.Cells().Select(c => c.GetString()); is quite insufficient for column-wide ranges (e.g. A:B).

Yeah, was about to discuss that with you on WhatsApp. I think using CellRangeReference.GetEnumerator() would be better, right?

James-Whitfield and others added some commits Oct 4, 2018

started to add textjoin
finished tests and func to match excel implementation

noticed the max params specified was incorrect

implemented testcases and moved test into correct file

fixes for PR comments.

removed explicit var type.
code maid clean up files.

@Pankraty Pankraty force-pushed the James-Whitfield:feature/#1010-text-join-func branch from 75f5dc0 to 789ffec Oct 15, 2018

@igitur

igitur approved these changes Jan 10, 2019

@igitur igitur added this to the v0.95 milestone Jan 10, 2019

@igitur igitur added the enhancement label Jan 10, 2019

@igitur igitur merged commit 31fc350 into ClosedXML:develop Jan 10, 2019

2 checks passed

WIP Legacy commit status override — see details
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@igitur

This comment has been minimized.

Copy link
Member

igitur commented Jan 10, 2019

Thanks @James-Whitfield and @Pankraty for the contributions.

@igitur igitur referenced this pull request Jan 10, 2019

Closed

Formula not supported: TEXTJOIN() #1010

1 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment