Skip to content

Conversation

@reddyashish
Copy link
Contributor

Purpose

DYN-9407

Handling PhoneticRun elements for excel nodes (https://learn.microsoft.com/en-us/dotnet/api/documentformat.openxml.spreadsheet.phoneticrun?view=openxml-3.0.1). These characters should not be displayed when using OpenXMLImportExcel node.

Declarations

Check these if you believe they are true

Release Notes

[DYN-9407] Handle PhoneticRun elements for import-excel node

Reviewers

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9407

Copy link
Contributor

@aparajit-pratap aparajit-pratap left a comment

Choose a reason for hiding this comment

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

Can you add a test case?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds logic to exclude PhoneticRun elements (phonetic guides) from extracted shared string values when importing Excel via OpenXML.

  • Replaces direct InnerText access with a new helper that filters out phonetic runs.
  • Introduces GetSharedStringText and IsInsidePhoneticRun helper methods to process SharedStringItem contents.

if (cell.CellValue.TryGetInt(out var index))
{
return sharedStringTable.ElementAt(index).InnerText;
return GetSharedStringText(sharedStringTable.ElementAt(index));
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

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

sharedStringTable.ElementAt(index) will iterate to the index each call; if this method executes for many cells, prefer direct indexed access (e.g., sharedStringTable.ChildElements[index] or caching Elements().ToList()) to avoid repeated enumeration.

Copilot uses AI. Check for mistakes.
@reddyashish
Copy link
Contributor Author

@aparajit-pratap added the test and addressed comments.

@reddyashish
Copy link
Contributor Author

@reddyashish reddyashish merged commit cbc5d68 into DynamoDS:master Oct 21, 2025
18 of 20 checks passed
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