-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Improve usability of "enable nullable reference types" refactoring #58316
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
Conversation
|
|
||
| // Only the input solution contains '#nullable enable' | ||
| if (!document.GetTextSynchronously(CancellationToken.None).ToString().Contains("#nullable enable")) | ||
| if (!Regex.IsMatch(document.GetTextSynchronously(CancellationToken.None).ToString(), "#nullable ?enable")) |
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.
❓My regex fu is limited. Is this matching 0 or 1 spaces before enable? Or is the ? applied to enable somehow?
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.
yeah, i don't quite get the need for this. afaict, this is allowing for either one or two spaces after #nullable. Is that really needed?
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.
maybe + or \s+ if you want to match any amount of whitespace?
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.
Spaces between # and nullable are allowed by the compiler (it's a test code anyway, so probably doesn't matter).
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.
afaict, this is allowing for either one or two spaces after #nullable. Is that really needed?
Yes. All of the tests have one space except for one test that has two spaces. I'll update the comment.
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.
➡️ Comment is now updated
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.
This whole method is a workaround for the fact that the test library can't cleanly represent a code action that changes compilation options. Considering a few options and this will be updated after.
92787bd to
b885da9
Compare
See #57476