| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 666
DYN-9196 switching to a DS-specific escaper #16501
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
Conversation
chubakueno
left a comment
chubakueno
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't you just use private string GetEscapedString(string s) from the Parser like you said?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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-9196
Labels
4 participants
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
@ luis.rivera@autodesk.com - not sure about Luis' handle
@QilongTang
FYIs
@saintentropy