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

feat: handle invalid xml characters #89

Conversation

NikiforovAll
Copy link
Contributor

@NikiforovAll NikiforovAll commented Nov 21, 2024

This pull request includes several changes to the Clippit.Tests/Excel/Samples/SpreadsheetWriterSamples.cs and Clippit/Excel/SpreadsheetWriter.cs files to improve code readability, initialization, and XML handling. The most important changes include the removal of unnecessary using statements, the conversion of array initializations to collection initializations, and the addition of a method to sanitize XML strings.

Code readability and initialization improvements:

XML handling improvements:

we want to preserve previous behavior that throws exception
@sergey-tihon
Copy link
Owner

  1. Please take a look at failed build
  2. Copilot found typos in the code and propose some nice suggestions
image
  1. Why do we need to introduce option and choose between exception and removal? Why we cannot properly encode invalid symbols and keep them in Excel?
private static string SanitizeXmlString(string? xml)
{
    if (string.IsNullOrEmpty(xml))
    {
        return string.Empty;
    }

    var buffer = new StringBuilder(xml.Length);

    foreach (var c in xml)
    {
        if (XmlConvert.IsXmlChar(c))
        {
            buffer.Append(c);
        }
        else
        {
            buffer.AppendFormat("&#x{0:X};", (int)c); // Encode invalid characters
        }
    }

    return buffer.ToString();
}

@sergey-tihon sergey-tihon added the Excel Excel related tasks label Nov 21, 2024
@NikiforovAll
Copy link
Contributor Author

How did you generate copilot suggestions?

@sergey-tihon
Copy link
Owner

How did you generate copilot suggestions?

I have Copilot integrated into GitHub
https://docs.github.com/en/enterprise-cloud@latest/copilot/using-github-copilot/asking-github-copilot-questions-in-github

@NikiforovAll
Copy link
Contributor Author

I actually, have it too. Thank you for the tip

@sergey-tihon sergey-tihon merged commit 6f9e4b0 into sergey-tihon:master Nov 22, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Excel Excel related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants