Stop duplicating trailing period in IL2026 #2053
Conversation
| @@ -113,7 +113,10 @@ protected override bool ReportSpecialIncompatibleMembersDiagnostic (OperationAna | |||
| protected override string GetMessageFromAttribute (AttributeData? requiresAttribute) | |||
| { | |||
| var message = requiresAttribute?.NamedArguments.FirstOrDefault (na => na.Key == "Message").Value.Value?.ToString (); | |||
| return string.IsNullOrEmpty (message) ? "" : $" {message}."; | |||
| if (!string.IsNullOrEmpty (message)) | |||
| message = $" {message}{(message!.TrimEnd ().EndsWith (".") ? "" : ".")}"; | |||
There was a problem hiding this comment.
Should we just stop printing a period all-together? Examining the message string used by the code feels like an anti-pattern
There was a problem hiding this comment.
I don't have a personal opinion on this one. I agree that examining the message string feels a bit wrong, so we can stop adding the period.
There was a problem hiding this comment.
That would be my preferred solution. @eerhardt Any objections?
There was a problem hiding this comment.
Note that there are a lot of existing warning messages without a trailing period:
If we decide to not append a period, we should have a plan for those.
There was a problem hiding this comment.
My plan: so what? 😄 The lack of a period at the end doesn't really look bad to me.
There was a problem hiding this comment.
Basically, I think it's important that messages from our tools don't look sloppy because it might affect how people perceive the tool (sloppy messages -> sloppy tool -> sloppy .NET).
It would be nice if we can solve this for all messages - not just our own. But if we go with fixing just our own, I think it would be really nice if there at least was a Roslyn analyzer for that, if it's not too hard to make one.
There was a problem hiding this comment.
It sounds like we're converging towards the solution in this PR: always add a period, and remove any duplicates. Any objections?
There was a problem hiding this comment.
always add a period, and remove any duplicates
So basically the changes implemented in this PR?
The other option is to stop looking into the user-given string (needed to remove duplicates) and handle the responsibility of adding (or not -- depending on the language of the user) a period to the message.
It would be nice if we can solve this for all messages - not just our own. But if we go with fixing just our own, I think it would be really nice if there at least was a Roslyn analyzer for that, if it's not too hard to make one.
I agree we should have an analyzer do some basic string formatting validation for things that will end up being read by the user.
There was a problem hiding this comment.
Yup, what's in this PR looks good. @mateoatr thanks for hosting this discussion 😄
* Don't duplicate trailing periods on IL2026 warning message * Do the same for RAF * Run lint.cmd Commit migrated from dotnet/linker@0f3ea71
Fixes #2052.