Add support to escape/unescape testcase filter strings #1627
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| namespace Microsoft.VisualStudio.TestPlatform.Common.Filtering | ||
| { | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Globalization; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using Microsoft.VisualStudio.TestPlatform.Common.Resources; | ||
|
|
||
| public static class FilterHelpers | ||
| { | ||
| private const char EscapePrefix = '%'; | ||
|
There was a problem hiding this comment. Can we use There was a problem hiding this comment. I agree with you that this is hard to use in CLI scenario. As you can see from the out-of-sync comment you pointed out below, I actually started with exactly what you suggested, but quickly realized that it would involve a bigger change, which I was reluctant to do this late and I really want this fix for preview 3, as it's a serious regression for LUT. Let me give this a try today and see how it goes. |
||
| private static readonly Dictionary<char, char> LookUpMap = | ||
| new Dictionary<char, char>() | ||
| { | ||
| { '%', '0'}, | ||
|
There was a problem hiding this comment. Is this exhaustive list of special characters we expect ? If not, this won't scale as we can add just 2 more with current design as %9 will be the last supported value. There was a problem hiding this comment. I don't know if there's plan to expand operator/operation support for filter, but we can always use "%a", "%b", etc |
||
| { '(', '1'}, | ||
| { ')', '2'}, | ||
| { '&', '3'}, | ||
| { '|', '4'}, | ||
| { '=', '5'}, | ||
| { '!', '6'}, | ||
| { '~', '7'}, | ||
| }; | ||
|
|
||
| private static readonly Dictionary<char, char> ReverseLookUpMap = LookUpMap.ToDictionary(kvp => kvp.Value, kvp => kvp.Key); | ||
| private static readonly char[] SpecialCharacters = LookUpMap.Keys.ToArray(); | ||
|
|
||
| /// <summary> | ||
| /// Escapes a set of special characters for filter (%, (, ), &, |, =, !, ~) by replacing them with their escape sequences. | ||
| /// </summary> | ||
| /// <param name="str">The input string that contains the text to convert.</param> | ||
| /// <returns>A string of characters with special characters converted to their escaped form.</returns> | ||
| public static string Escape(string str) | ||
|
There was a problem hiding this comment. I am guessing. we are just going to replicate this code on the IDE side. If yes, it is problematic, maintenance issues and what not. There was a problem hiding this comment. I made the helper class public, so it can be used by other components. But I'm not sure if this is the right place for public helpers. |
||
| { | ||
| if (str == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(str)); | ||
| } | ||
|
|
||
| if (str.IndexOfAny(SpecialCharacters) < 0) | ||
| { | ||
| return str; | ||
| } | ||
|
|
||
| var builder = new StringBuilder(); | ||
| for (int i = 0; i < str.Length; ++i) | ||
| { | ||
| var currentChar = str[i]; | ||
| if (LookUpMap.TryGetValue(currentChar, out var escaped)) | ||
| { | ||
| builder.Append(EscapePrefix); | ||
| currentChar = escaped; | ||
| } | ||
| builder.Append(currentChar); | ||
| } | ||
|
|
||
| return builder.ToString(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Converts any escaped characters in the input filter string. | ||
| /// </summary> | ||
| /// <param name="str"></param> | ||
|
|
||
| /// <returns>A filter string of characters with any escaped characters converted to their unescaped form.</returns> | ||
| public static string Unescape(string str) | ||
| { | ||
| if (str == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(str)); | ||
| } | ||
|
|
||
| if (str.IndexOf(EscapePrefix) < 0) | ||
| { | ||
| return str; | ||
| } | ||
|
|
||
| var builder = new StringBuilder(); | ||
| for (int i = 0; i < str.Length; ++i) | ||
| { | ||
| var currentChar = str[i]; | ||
| if (currentChar == EscapePrefix) | ||
| { | ||
| if (++i == str.Length || !ReverseLookUpMap.TryGetValue(currentChar = str[i], out var specialChar)) | ||
| { | ||
| // "\" should be followed by a valid escape seq. i.e. '0' - '7'. | ||
|
|
||
| throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, Resources.TestCaseFilterEscapeException, str)); | ||
| } | ||
| currentChar = specialChar; | ||
| } | ||
|
|
||
| // Strictly speaking, string to be unescaped shouldn't contain any of the special characters (except '%'), | ||
| // but we will ignore that to avoid additional overhead. | ||
|
|
||
| builder.Append(currentChar); | ||
| } | ||
|
|
||
| return builder.ToString(); | ||
| } | ||
| } | ||
| } | ||
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.
Can this be internal? LUT has dependency on
Microsoft.TestPlatform.Common?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.
Yes, made this public for LUT. LUT has dependency on TranslationLayer, which pulls in Common as well.
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.
Got it. However not all clients(VS code, Rider,..) uses TranslationLayer. We should rename this to FilterEscapeHelper and move this to OM?
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.
I guess the tokenizer code should remain private to FilterExpression/Condition in this case, don't see a point of exposing them.