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

Escape special characters in section name of editor config #52515

Merged
merged 5 commits into from
Apr 14, 2021

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Apr 8, 2021

Addresses part of #52469.

@captainsafia captainsafia requested a review from chsienki April 8, 2021 22:31
@captainsafia captainsafia requested a review from a team as a code owner April 8, 2021 22:31
@jcouv jcouv added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers and removed Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Apr 9, 2021
@Youssef1313
Copy link
Member

I added the ConfigWithEscapedValues in AnalyzerConfigTests to validate that the SectionNameLexer correctly processes the escaped values but it doesn't seem like the Parse helper in the AnalyzerConfigTests actually calls this codepath? It looks like it stops processing the section name here.

I believe this code path is covered by when you call TryCreateSectionNameMatcher, which should be already covered in tests. For example this test:

[Fact]
public void LiteralBraces()
{
SectionNameMatcher matcher = TryCreateSectionNameMatcher("abc\\{\\}def").Value;
Assert.Equal(@"^.*/abc\{}def$", matcher.Regex.ToString());
Assert.True(matcher.IsMatch("/abc{}def"));
Assert.True(matcher.IsMatch("/subdir/abc{}def"));
Assert.False(matcher.IsMatch("/abcdef"));
Assert.False(matcher.IsMatch("/abc}{def"));
}

{
builder.Append("\\");
}
builder.Append(c);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to the issue you're trying to solve, but does this need to escape slashes (\)?

Copy link
Member Author

Choose a reason for hiding this comment

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

For this scenario, I tried to stick to values that were defined as special characters per the editor config specification (http://docs.editorconfig.org/en/master/editorconfig-format.html).

Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@captainsafia captainsafia merged commit 1fb7242 into main Apr 14, 2021
@ghost ghost added this to the Next milestone Apr 14, 2021
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
@JoeRobich JoeRobich deleted the safia/fix-analyzer-config branch November 3, 2021 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants