DYN-9196 .NET10 Migration - switching to a DS-specific escaper#16501
DYN-9196 .NET10 Migration - switching to a DS-specific escaper#16501QilongTang merged 4 commits intoDynamoDS:masterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the old CodeDom-based string escaping implementation with a new DesignScript-specific escaper. The change addresses brittleness issues with .NET 10 and aligns with DesignScript documentation by only escaping backslashes and double quotes while preserving all other characters including Unicode and newlines.
- Replace CodeDom string literal generation with direct character-by-character escaping
- Add comprehensive test coverage for the new escaping logic
- Optimize performance with a fast path for strings that don't need escaping
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Engine/ProtoCore/Utils/CompilerUtils.cs | Replaces CodeDom-based ToLiteral method with DesignScript-specific escaper |
| test/Engine/ProtoTest/UtilsTests/CompilerUtilsTest.cs | Adds comprehensive test suite covering edge cases, Unicode, performance, and round-trip validation |
Comments suppressed due to low confidence (1)
test/Engine/ProtoTest/UtilsTests/CompilerUtilsTest.cs:1
- The explicit handling of
\rand\ncharacters in the switch statement is unnecessary. These characters don't need escaping according to DesignScript rules, so they should be handled by the default case. The explicit cases add complexity without functional benefit.
using System;
| /// </summary> | ||
| /// <param name="input">The string to escape</param> | ||
| /// <returns>The escaped string suitable for DesignScript string literals</returns> | ||
| internal static string ToLiteral(string input) |
There was a problem hiding this comment.
Why can't you just use private string GetEscapedString(string s) from the Parser like you said?
There was a problem hiding this comment.
Hey @aparajit-pratap, thank you for the suggestion! The reason we can't directly use GetEscapedString is that it does the reverse of what we need - it unescapes strings (converts " to "), while ToLiteral needs to escape strings (converts " to ").
With that said, I've now reworked the ToLiteral method to closely follow what GetEscapedString does, but in reverse. Can you have a look and see if that's more what you had in mind?
- Ensure perfect round-trip compatibility with Parser
- change tests to align with aligning with the Parser.GetEscapedString inverse functionality
Since we are now following closely the Parser.GetEscapedString reverse functionality, I had to amend some of the old tests to reflect that. All should be done now. |
QilongTang
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9196
QilongTang
changed the title
Purpose
A follow up of https://github.com/DynamoDS/Dynamo/pull/16478/files#diff-d37d8c6fdae53b6093a8ac0f0e6198e21b99f65a1a60c9be8d39e6396444cd89.
This PR replaces the old ToLiteral implementation, which used CodeDom to generate C# string literals and then post-processed them.
The new implementation directly escapes only backslashes () and double quotes (") per DesignScript rules.
Resources used:
Dynamo/src/Engine/ProtoCore/Parser/Parser.cs
Line 101 in 6bc77bb
Declarations
Check these if you believe they are true
Release Notes
Reviewers
@ [email protected] - not sure about Luis' handle
@QilongTang
FYIs
@saintentropy