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

Make nullness equivalent warning message clearer. #18172

Merged
merged 3 commits into from
Jan 13, 2025

Conversation

isaacabraham
Copy link
Contributor

@isaacabraham isaacabraham commented Dec 22, 2024

Description

Fixes #18171. Warning now reads:

Nullness warning: A non-nullable 'String' was expected but this expression is nullable. Consider either changing the target to also be nullable, or use pattern matching to safely handle the null case of this expression.

Checklist

  • Test cases updated

NO_RELEASE_NOTES

@isaacabraham isaacabraham requested a review from a team as a code owner December 22, 2024 15:01
Copy link
Contributor

github-actions bot commented Dec 22, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@isaacabraham
Copy link
Contributor Author

Is this failing because of the lack of release notes or have I botched something else?

Also - what causes the BSL files to get regenerated? I ended up doing it by hand.

@majocha
Copy link
Contributor

majocha commented Dec 23, 2024

@isaacabraham looks like the only error apart from lack of release notes was this crash. If it passes locally it is most probably good.

@smoothdeveloper
Copy link
Contributor

@isaacabraham setting the environment variable TEST_UPDATE_BSL to 1 and running the test should update your .bsl files.

@dotnet dotnet deleted a comment from github-actions bot Dec 23, 2024
@isaacabraham
Copy link
Contributor Author

isaacabraham commented Dec 24, 2024

@isaacabraham setting the environment variable TEST_UPDATE_BSL to 1 and running the test should update your .bsl files.

By running build -TEST or is there a quicker way of doing it?

@smoothdeveloper
Copy link
Contributor

You can hijack the check of that variable:

if updateBaseline () then

match Environment.GetEnvironmentVariable("TEST_UPDATE_BSL") with

Or if you run the suite from command line, using set TEST_UPDATE_BSL=1 per the dev guidelines:

set TEST_UPDATE_BSL=1

@isaacabraham
Copy link
Contributor Author

@isaacabraham looks like the only error apart from lack of release notes was this crash. If it passes locally it is most probably good.

Any idea how to make this green?

@majocha
Copy link
Contributor

majocha commented Dec 25, 2024

Any idea how to make this green?

Just rerun the CI and hopefully the tests will not crash the CLR this time. To rerun you can close and reopen the PR or just add another commit with release notes. Sorry for the tests being a bit unstable in CI right now. I hope this will help when merged: #18169.

@dotnet dotnet deleted a comment from github-actions bot Dec 26, 2024
@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Dec 30, 2024
@isaacabraham
Copy link
Contributor Author

Any idea how to make this green?

Just rerun the CI and hopefully the tests will not crash the CLR this time. To rerun you can close and reopen the PR or just add another commit with release notes. Sorry for the tests being a bit unstable in CI right now. I hope this will help when merged: #18169.

For the release notes, what can I do to fix that - which file needs updating?

@psfinaki
Copy link
Member

psfinaki commented Jan 6, 2025

@isaacabraham no need for release notes here, the release notes check in this PR is already lifted and the CI is green :)

FYI, when there is a need for them, the respective comment points to the exact release note which requires an update.

@T-Gro T-Gro enabled auto-merge (squash) January 13, 2025 14:42
Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Right, this is a more helpful diagnostic, thanks!

@T-Gro T-Gro merged commit ec2b3d9 into dotnet:main Jan 13, 2025
33 checks passed
@isaacabraham isaacabraham deleted the nullness-equivalent-warning branch January 16, 2025 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Improve Error Reporting: Make Nullness Warning 3261 less intimidating - Part 2
5 participants