Skip to content

Conversation

@davidwengier davidwengier requested review from a team as code owners March 3, 2023 03:26
@davidwengier
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

var csharp = owner.FirstAncestorOrSelf<CSharpCodeBlockSyntax>();

return element.Body.Any(c => c is CSharpCodeBlockSyntax) || csharp is not null;
return element?.Body.Any(c => c is CSharpCodeBlockSyntax) ?? false || csharp is not null;
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 Is the ?? false necessary?

Suggested change
return element?.Body.Any(c => c is CSharpCodeBlockSyntax) ?? false || csharp is not null;
return element?.Body.Any(c => c is CSharpCodeBlockSyntax) ?? (csharp is not null);

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally I find that confusing, and would have to spend a few minutes trying to understand the logic.

Comment on lines 257 to +258
var element = owner.FirstAncestorOrSelf<MarkupElementSyntax>();
if (element is null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var element = owner.FirstAncestorOrSelf<MarkupElementSyntax>();
if (element is null)
if (owner.FirstAncestorOrSelf<MarkupElementSyntax>() is not { } element)

@davidwengier davidwengier merged commit 77001a1 into dotnet:main Mar 4, 2023
@davidwengier davidwengier deleted the FixNREInDiagnosticsTranslator branch March 4, 2023 01:02
@davidwengier
Copy link
Member Author

Whoops! Sorry, hit merge from my phone without thinking about compiler reviews. @333fred let me know if you want me to revert, though its pretty minor. There are callers of the method in the compiler, but none of the callers are annotated anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants