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

[wasm][debugger] Revert don't need to escape special characters anymore #78320

Merged
merged 15 commits into from
Dec 8, 2022

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Nov 14, 2022

If is a letter or a digit we don't need to escape which is the case of the swedish characters reported in #76777

@ghost
Copy link

ghost commented Nov 14, 2022

Tagging subscribers to this area: @thaystg
See info in area-owners.md if you want to be subscribed.

Issue Details

If is a letter or a digit we don't need to escape which is the case of the swedish characters reported in #76777

Author: thaystg
Assignees: -
Labels:

area-Debugger-mono

Milestone: -

@ilonatommy
Copy link
Member

Manual testing I've done - everything is OK:

  • with PR new version of tests pass, without change in PR they fail;
  • special char (#) in blazor using VS/Browser;
  • special char in wasmbrowser using VS/Browser;
  • diacritics in path (including Swedish) in blazor VS/Browser;
  • diacritics in path in browserwasm VS/Browser.
    I was checking if we can hit a bp.

@thaystg
Copy link
Member Author

thaystg commented Nov 18, 2022

Added the test case that will be executed on linux and here the screenshot to show it working correctly when debugging on chrome on Mac

image

@thaystg thaystg requested a review from radical November 18, 2022 21:35
Copy link
Member

@ilonatommy ilonatommy left a comment

Choose a reason for hiding this comment

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

LGTM, only I don't understand the details of the colon-project change.

}

[Fact]
Copy link
Member

@radical radical Nov 23, 2022

Choose a reason for hiding this comment

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

Can we use [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows))] here?

Copy link
Member

Choose a reason for hiding this comment

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

or [PlatformSpecific(TestPlatforms.AnyUnix)]

Copy link
Member Author

@thaystg thaystg Nov 24, 2022

Choose a reason for hiding this comment

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

It looks like it doesn't work, it's trying to run it on windows:

        [Fact]
        [PlatformSpecific(TestPlatforms.AnyUnix)]


         | [2022-11-24T14:57:59] DevToolsProxy-0 Debug: sending error response for id: msg-:::5 -> [Result: IsOk: False, IsErr: True, Value: , Error: {
 |                                                "message": "Assembly 'debugger-test-with-colon-in-source-name.dll' not found,needed to get method location of 'DebuggerTests.CheckColonInSourceName:Evaluate'"
 |                                              } ]

Copy link
Member Author

Choose a reason for hiding this comment

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

Same error with this option:

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows))]

Copy link
Member Author

Choose a reason for hiding this comment

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

Right: it looks like this one will work: [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsLinux))]

private static string EscapeAscii(string path)
{
var builder = new StringBuilder();
foreach (var part in Regex.Split(path, @"([:/])"))
Copy link
Member

Choose a reason for hiding this comment

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

The : is needed only for windows. And for !windows, we can do a regular string split on /.
If we must use regex here, then maybe move it to a static readonly field.

@thaystg thaystg merged commit 2480f01 into dotnet:main Dec 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants