Skip to content

Fix some warnings/nullable inconsistencies. #1451

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

MissedTheMark
Copy link
Contributor

@MissedTheMark MissedTheMark commented Mar 12, 2025

Background

Calamari has a whole bunch of build warnings, and nullable things that are inconsistent/misleading.

The PR endeavors to fix some of these problems.

How to review

For once, my commits are well named, and each contains a specific change across files, or changes in a single file. It would probably be best to go through each commit one at a time, using the commit message for context, rather than attempting to review the whole PR without context.

@MissedTheMark MissedTheMark marked this pull request as ready for review March 12, 2025 06:23
@MissedTheMark MissedTheMark changed the title Fix some warnings/nullable stuff. Fix some warnings/nullable inconsistencies. Mar 12, 2025
@@ -10,6 +10,7 @@ namespace Calamari.Common.Features.Deployment.Journal
{
public class DeployedPackage
{
#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor - Handled by guard method.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused - the API allows null - but then we bounce it via the Guard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's because when using the XElement constructor, the PackageId or PackageVersion might come out as null, and this gives you a 'nice' message?

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.

2 participants