Skip to content

Conversation

@zhiyuanliang-ms
Copy link
Member

@zhiyuanliang-ms zhiyuanliang-ms commented Jun 18, 2024

Why this PR?

Fix xUnit1031 warning.

Remove nullable property in example projects.

Visible changes

Update testcases to use async&await.

Update property.

@zhiyuanliang-ms zhiyuanliang-ms changed the title Fix xunit1031 warning Fix build warning Jun 18, 2024
@zhiyuanliang-ms
Copy link
Member Author

/AzurePipeline run

@rossgrambo
Copy link
Member

Incorrect nullable

What makes the nullable incorrect here?

@zhiyuanliang-ms
Copy link
Member Author

zhiyuanliang-ms commented Jun 19, 2024

What makes the nullable incorrect here?

@rossgrambo I am trying to fix these build warnings:

Microsoft.FeatureManagement -> C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\src\Microsoft.FeatureManagement\bin\Debug\netstandard2.1\Microsoft.FeatureManagement.dll
C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\TargetingConsoleApp\Identity\User.cs(10,23): warning CS8618: Non-nullable property 'Id' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. [C:\Users\zhiyuanliang\OneDri
ve - Microsoft\Desktop\Dev\FM\examples\TargetingConsoleApp\TargetingConsoleApp.csproj]
C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\TargetingConsoleApp\Identity\User.cs(12,36): warning CS8618: Non-nullable property 'Groups' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. [C:\Users\zhiyuanliang\On
eDrive - Microsoft\Desktop\Dev\FM\examples\TargetingConsoleApp\TargetingConsoleApp.csproj]
C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\TargetingConsoleApp\Identity\InMemoryUserRepository.cs(94,20): warning CS8619: Nullability of reference types in value of type 'Task<User?>' doesn't match target type 'Task'. [C:\Users\zhiyuanliang\OneDrive - Mic
rosoft\Desktop\Dev\FM\examples\TargetingConsoleApp\TargetingConsoleApp.csproj]
C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\Models\ErrorViewModel.cs(10,23): warning CS8618: Non-nullable property 'RequestId' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. [C:\Users\zhiyuanl
iang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\FeatureFlagDemo.csproj]
C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\HttpContextTargetingContextAccessor.cs(28,39): warning CS8600: Converting null literal or possible null value to non-nullable type. [C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\Fe
atureFlagDemo\FeatureFlagDemo.csproj]
C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\HttpContextTargetingContextAccessor.cs(32,17): warning CS8602: Dereference of a possibly null reference. [C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\FeatureFlagDe
mo.csproj]
C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\HttpContextTargetingContextAccessor.cs(32,75): warning CS8600: Converting null literal or possible null value to non-nullable type. [C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\Fe
atureFlagDemo\FeatureFlagDemo.csproj]
C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\HttpContextTargetingContextAccessor.cs(34,56): warning CS8600: Converting null literal or possible null value to non-nullable type. [C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\Fe
atureFlagDemo\FeatureFlagDemo.csproj]
C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\HttpContextTargetingContextAccessor.cs(34,56): warning CS8604: Possible null reference argument for parameter 'result' in 'ValueTask.ValueTask(TargetingContext result)'. [C:\Users\zhiy
uanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\FeatureFlagDemo.csproj]
C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\HttpContextTargetingContextAccessor.cs(55,26): warning CS8602: Dereference of a possibly null reference. [C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\FeatureFlagDe
mo.csproj]
C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\BrowserFilter.cs(44,32): warning CS8602: Dereference of a possibly null reference. [C:\Users\zhiyuanliang\OneDrive - Microsoft\Desktop\Dev\FM\examples\FeatureFlagDemo\FeatureFlagDemo.csproj]

There is inconsistent usage of nullable type. There are two options to fix these build warnings:

  1. Keep the nullable property in csproj file and add question marks for the type where the warnings occur.
  2. Remove the nullable property in csproj file and stop using nullable type.

I prefer the second option.

@zhiyuanliang-ms
Copy link
Member Author

/AzurePipeline run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rossgrambo
Copy link
Member

I see. We're setting nullable true but not using the nullable ? everywhere where applicable.

<nullable>true</nullable> is automatically added in new projects. I'd prefer we keep it and add the question marks where needed.

@zhiyuanliang-ms
Copy link
Member Author

zhiyuanliang-ms commented Jun 20, 2024

@rossgrambo

I'd prefer we keep it and add the question marks where needed.

Then, too many changes are needed, especially for FeatureFlagDemo project. I don't think it's worthy because our feature management projects don't set property to true. Example projects just follow them.

@zhiyuanliang-ms
Copy link
Member Author

@rossgrambo
I will follow this PR #305
I will revert the change for RazorPages but keep the changes for other example projects.

@zhiyuanliang-ms zhiyuanliang-ms merged commit 95e7833 into main Jun 20, 2024
@zhiyuanliang-ms zhiyuanliang-ms deleted the zhiyuanliang/fix-xunit1031 branch June 20, 2024 05:42
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.

4 participants