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.

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

feat: Added Nullability markup for some classes #1710

Merged
merged 11 commits into from
Oct 8, 2024

Conversation

HavenDV
Copy link
Contributor

@HavenDV HavenDV commented Jun 27, 2024

Related to #1202

I think before that, #1202 should be split into two parts - marking the current code as is without changing the behavior
And after that, for the next big update, REQUIRED things should be replaced so that they are not null even in an empty document

This PR does not change behavior in places where null guards were provided or null was expected.
In places where there were NREs, ? were added. or other trivial fixes to avoid NRE
I'll continue with nullable markup if the change is accepted quickly enough

@darrelmiller
Copy link
Member

We reviewed these changes and I think we understand the incremental step you are trying to take. This is consistent with the direction that we want to go for the v2 release. Thank you for making these changes. Be aware that there are likely going to still be a fair amount of code churn in this branch, so the kind of changes that still need to be made may cause merge conflicts.

@HavenDV
Copy link
Contributor Author

HavenDV commented Jul 16, 2024

It seems we need to update System.Text.Json to 8.0.4 because

/home/runner/work/OpenAPI.NET/OpenAPI.NET/src/Microsoft.OpenApi/Microsoft.OpenApi.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 7.0.2 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w [/home/runner/work/OpenAPI.NET/OpenAPI.NET/Microsoft.OpenApi.sln]
/home/runner/work/OpenAPI.NET/OpenAPI.NET/src/Microsoft.OpenApi.Readers/Microsoft.OpenApi.Readers.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 7.0.2 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w [/home/runner/work/OpenAPI.NET/OpenAPI.NET/Microsoft.OpenApi.sln]

It doesn't allow to pass CICD. After the update, some of the tests fail, so this needs to be done in a separate PR with update/fix for tests
Or I can add NoWarn for this temporarily.

@baywet
Copy link
Member

baywet commented Jul 16, 2024

@HavenDV good call.
Can you add the suppression in a similar way as this PR please? microsoft/kiota-dotnet#292

And make sure we're using version ranges for library projects.

Thanks a lot!

@baywet
Copy link
Member

baywet commented Jul 19, 2024

Quote reply
Reference in ne

Actually this has already been handled in #1729

@HavenDV
Copy link
Contributor Author

HavenDV commented Jul 19, 2024

Yes, I saw this so I decided not to risk making parallel changes here.
I'll have some free time today/weekend, I might take this further.

@HavenDV HavenDV requested a review from baywet July 30, 2024 13:13
@HavenDV
Copy link
Contributor Author

HavenDV commented Aug 5, 2024

@baywet I'm not sure, but maybe it's blocked by "baywet requested changes" status. Is there anything else that needs fixing?

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Sorry, I forgot to reset my review

@darrelmiller
Copy link
Member

@MaggieKimani1 Could we get this PR merged before we get further conflicts?

# Conflicts:
#	src/Microsoft.OpenApi/Models/OpenApiComponents.cs
#	src/Microsoft.OpenApi/Models/OpenApiDocument.cs
#	src/Microsoft.OpenApi/Models/OpenApiMediaType.cs
#	test/Microsoft.OpenApi.Hidi.Tests/Formatters/PowerShellFormatterTests.cs
#	test/Microsoft.OpenApi.Tests/PublicApi/PublicApi.approved.txt
@MaggieKimani1 MaggieKimani1 merged commit d302b5d into microsoft:release/2.0.0 Oct 8, 2024
6 checks passed
@HavenDV HavenDV deleted the nullability-work-1 branch October 8, 2024 21:09
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