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

Jump to bottom

Consistently use 'is null' and 'is not null' iso '== null' and '!= null' #4379

Merged
merged 2 commits into from
May 6, 2023
Merged

Consistently use 'is null' and 'is not null' iso '== null' and '!= null' #4379

merged 2 commits into from
May 6, 2023

Conversation

manfred-brands
Copy link
Member

Fixes #4378

mikkelbu
mikkelbu approved these changes May 5, 2023
View reviewed changes
Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @manfred-brands. I only have some nit-picks (mostly relating to the formatting of ternary expressions after the update) - otherwise it looks good.

src/NUnitFramework/tests/TestUtilities/Comparers/TestComparer.cs Outdated
return 0;

if (x == null || y == null)
if (x is null || y is null)
Copy link
Member

@mikkelbu mikkelbu May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unrelated to your change, but this is an odd (read: wrong) IComparer

src/NUnitFramework/tests/Internal/RuntimeFrameworkTests.cs Outdated
@@ -11,8 +11,7 @@ public class RuntimeFrameworkTests
static readonly RuntimeType currentRuntime = RuntimeType.NetCore;
#else
static readonly RuntimeType currentRuntime =
Type.GetType("Mono.Runtime", false) != null
? RuntimeType.Mono
Type.GetType("Mono.Runtime", false) is not null ? RuntimeType.Mono
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Type.GetType("Mono.Runtime", false) is not null ? RuntimeType.Mono
Type.GetType("Mono.Runtime", false) is not null
? RuntimeType.Mono

src/NUnitFramework/nunitlite/TextRunner.cs Outdated
@@ -205,15 +205,14 @@ private int Execute()

_textUI.DisplayRuntimeEnvironment();

var testFile = _testAssembly != null
? AssemblyHelper.GetAssemblyPath(_testAssembly)
var testFile = _testAssembly is not null ? AssemblyHelper.GetAssemblyPath(_testAssembly)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var testFile = _testAssembly is not null ? AssemblyHelper.GetAssemblyPath(_testAssembly)
var testFile = _testAssembly is not null
? AssemblyHelper.GetAssemblyPath(_testAssembly)

src/NUnitFramework/nunitlite/TextRunner.cs Outdated
? _testAssembly.GetName().Name
: _options.InputFile != null
? Path.GetFileNameWithoutExtension(_options.InputFile)
var baseName = _testAssembly is not null ? _testAssembly.GetName().Name
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep the existing formatting

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 certainly agree, it seems the CodeFix from the Analyzer looses trailing trivia. I will create an issue (and PR) for it: AArnott/CSharpIsNull#45

src/NUnitFramework/framework/Internal/TestNameGenerator.cs Outdated
@@ -199,8 +199,7 @@ protected static void AppendGenericTypeNames(StringBuilder sb, MethodInfo method

protected static string GetDisplayString(object? arg, int stringMax)
{
string display = arg == null
? "null"
string display = arg is null ? "null"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
string display = arg is null ? "null"
string display = arg is null
? "null"

src/NUnitFramework/framework/Internal/Tests/TestAssembly.cs Outdated
@@ -67,8 +67,7 @@ public TestAssembly(TestAssembly assembly, ITestFilter filter)
/// </summary>
public override TAttr[] GetCustomAttributes<TAttr>(bool inherit)
{
return Assembly != null
? Assembly.GetAttributes<TAttr>()
return Assembly is not null ? Assembly.GetAttributes<TAttr>()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return Assembly is not null ? Assembly.GetAttributes<TAttr>()
return Assembly is not null
? Assembly.GetAttributes<TAttr>()

src/NUnitFramework/framework/Internal/Execution/EventPump.cs Outdated
@@ -150,7 +150,7 @@ private void PumpThreadProc()
while (true)
{
Event? e = _events.Dequeue( PumpState == EventPumpState.Pumping );
if ( e == null )
if ( e is null)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ( e is null)
if (e is null)

src/NUnitFramework/framework/Interfaces/TNode.cs Outdated
@@ -373,8 +373,7 @@ private static List<TNode> ApplySelection(List<TNode> nodeList, string xpath)
}
}

return tail != null
? ApplySelection(resultNodes, tail)
return tail is not null ? ApplySelection(resultNodes, tail)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return tail is not null ? ApplySelection(resultNodes, tail)
return tail is not null
? ApplySelection(resultNodes, tail)

src/NUnitFramework/framework/Attributes/TestCaseSourceAttribute.cs Outdated
@@ -159,11 +159,10 @@ private IEnumerable<ITestCaseData> GetTestCasesFor(IMethodInfo method)
// single null argument will cause an error to be
// reported at the test level, in most cases.
// 2. User provided an ITestCaseData and we just use it.
ITestCaseData? parms = item == null
? new TestCaseParameters(new object?[] { null })
ITestCaseData? parms = item is null ? new TestCaseParameters(new object?[] { null })
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ITestCaseData? parms = item is null ? new TestCaseParameters(new object?[] { null })
ITestCaseData? parms = item is null
? new TestCaseParameters(new object?[] { null })

@manfred-brands
Copy link
Member Author

@mikkelbu I have fixed the CSharpIsNullAnalyzer CodeFix (locally) and re-applied the codefix. Now the existing formatting has been kept.
Once the fix is officially released we can update our reference.

@manfred-brands manfred-brands merged commit 8a56c85 into nunit:master May 6, 2023
@manfred-brands manfred-brands deleted the UseIsNull branch May 6, 2023 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikkelbu mikkelbu mikkelbu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update code base to use 'is (not) null' consistently
2 participants
@manfred-brands @mikkelbu