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.

Sign up for GitHub

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

EditorConfig comments cannot follow a property value #51625

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

EditorConfig comments cannot follow a property value #51625

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b98df1
EditorConfig comments cannot follow a property value
sharwell Mar 3, 2021
1b6c125
Update parsing of dotnet_diagnostic lines to ignore "trailing comments"
sharwell Mar 3, 2021
ea18e2f
Merge remote-tracking branch 'dotnet/main' into no-comment
sharwell Jan 5, 2024
d797c32
Update tests and breaking change documentation
sharwell Jan 5, 2024
bf03a75
Update CodeStyleOption<T> parsing to ignore trailing comments
sharwell Jan 8, 2024
be49e52
Merge remote-tracking branch 'dotnet/main' into no-comment
sharwell Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,15 +177,18 @@ public void SpacesInProperties()
properties);
}

[Fact]
public void EndOfLineComments()
[Theory]
Copy link
Member

@jcouv jcouv Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Please document the breaking change in https://github.com/dotnet/roslyn/blob/main/docs/compilers/CSharp/Compiler%20Breaking%20Changes%20-%20post%20DotNet%205.md #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ Documented for .NET 9

[WorkItem(44596, "https://github.com/dotnet/roslyn/issues/44596")]
[InlineData(";")]
[InlineData("#")]
public void EndOfLineComments(string commentStartCharacter)
{
var config = ParseConfigFile(@"
my_prop2 = my val2 # Comment");
var config = ParseConfigFile($@"
my_prop2 = my val2 {commentStartCharacter} Not A Comment");

var properties = config.GlobalSection.Properties;
AssertEx.SetEqual(
new[] { KeyValuePair.Create("my_prop2", "my val2") },
new[] { KeyValuePair.Create("my_prop2", $"my val2 {commentStartCharacter} Not A Comment") },
properties);
}

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public sealed partial class AnalyzerConfig
// Matches EditorConfig section header such as "[*.{js,py}]", see https://editorconfig.org for details
private static readonly Regex s_sectionMatcher = new Regex(@"^\s*\[(([^#;]|\\#|\\;)+)\]\s*([#;].*)?$", RegexOptions.Compiled);
// Matches EditorConfig property such as "indent_style = space", see https://editorconfig.org for details
private static readonly Regex s_propertyMatcher = new Regex(@"^\s*([\w\.\-_]+)\s*[=:]\s*(.*?)\s*([#;].*)?$", RegexOptions.Compiled);
private static readonly Regex s_propertyMatcher = new Regex(@"^\s*([\w\.\-_]+)\s*[=:]\s*(.*?)\s*$", RegexOptions.Compiled);
sharwell marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

[=:]

Why are we considering both = and : to separate the key and the value? I don't think key : value should be allowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ I have no idea, but that seems beyond the scope of this change.

Copy link
Contributor

@ToddGrun ToddGrun Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Probably outside the scope of this PR, but should we be allowing whitespace within the key?

eg:
test value = allowed

Copy link
Member Author

Choose a reason for hiding this comment

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

It appears that we do not currently support whitespace inside the key, but according to the specification it should be allowed:

Key: The part before the first = (trimmed of whitespace, but including any whitespace in the middle).

Can you file a new issue for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

#71727


/// <summary>
/// Key that indicates if this config is a global config
Expand Down
19 changes: 13 additions & 6 deletions src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,33 +337,40 @@ static void freeKey(List<Section> sectionKey, ObjectPool<List<Section>> pool)

internal static bool TryParseSeverity(string value, out ReportDiagnostic severity)
{
ReadOnlySpan<char> commentStartCharacters = stackalloc char[] { ';', '#' };
Copy link
Member

Choose a reason for hiding this comment

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

nit: consider leaving a comment ("for backcompat, ...")

var trimmed = value.AsSpan();
var commentStartIndex = trimmed.IndexOfAny(commentStartCharacters);
if (commentStartIndex >= 0)
trimmed = trimmed[0..commentStartIndex].TrimEnd();
Copy link
Member

@jcouv jcouv Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I understand, this change is to maintain the existing behavior and I assume we already have tests covering this. Is that right? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ Yes, and tests have now been added.


var comparer = StringComparer.OrdinalIgnoreCase;
if (comparer.Equals(value, "default"))
if (trimmed.Equals("default".AsSpan(), StringComparison.OrdinalIgnoreCase))
Copy link
Member

Choose a reason for hiding this comment

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

The comparer local on line above (339) no longer seems to be used

{
severity = ReportDiagnostic.Default;
return true;
}
else if (comparer.Equals(value, "error"))
else if (trimmed.Equals("error".AsSpan(), StringComparison.OrdinalIgnoreCase))
{
severity = ReportDiagnostic.Error;
return true;
}
else if (comparer.Equals(value, "warning"))
else if (trimmed.Equals("warning".AsSpan(), StringComparison.OrdinalIgnoreCase))
{
severity = ReportDiagnostic.Warn;
return true;
}
else if (comparer.Equals(value, "suggestion"))
else if (trimmed.Equals("suggestion".AsSpan(), StringComparison.OrdinalIgnoreCase))
{
severity = ReportDiagnostic.Info;
return true;
}
else if (comparer.Equals(value, "silent") || comparer.Equals(value, "refactoring"))
else if (trimmed.Equals("silent".AsSpan(), StringComparison.OrdinalIgnoreCase)
|| trimmed.Equals("refactoring".AsSpan(), StringComparison.OrdinalIgnoreCase))
{
severity = ReportDiagnostic.Hidden;
return true;
}
else if (comparer.Equals(value, "none"))
else if (trimmed.Equals("none".AsSpan(), StringComparison.OrdinalIgnoreCase))
{
severity = ReportDiagnostic.Suppress;
return true;
Expand Down