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

Miscellaneous cleanup/throw helpers #1491

Merged
merged 5 commits into from
Sep 19, 2024
Merged

Conversation

Rob-Hague
Copy link
Collaborator

These are some random changes I've accumulated in some branches, which I've extracted out to reduce diffs & conflicts. They are roughly:

Add a reference to PolySharp to allow nullable attributes and CallerArgumentExpressionAttribute etc.
Add a ThrowHelpers class
Add #nullable to PrivateKeyFile and cleanup PrivateKeyFileTest
Also change around some of the newly added keys - I have a use for this with certificate support.

@Rob-Hague
Copy link
Collaborator Author

Ok to get a sign-off here?

@@ -32,7 +32,7 @@
Disable nullable warnings on old frameworks because of missing annotations.
-->
<PropertyGroup Condition=" !$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net6.0')) ">
<NoWarn>$(NoWarn);CS8602</NoWarn>
<NoWarn>$(NoWarn);CS8602;CS8604;CS8777</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

Not possible to fix these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are coming from string.IsNullOrEmpty and friends which are not annotated for nullable analysis on lower targets

@@ -188,3 +188,6 @@ dotnet_diagnostic.MA0040.severity = none
# MA0042: Do not use blocking calls in an async method
# duplicate of CA1849
dotnet_diagnostic.MA0042.severity = none

# S3236: Caller information arguments should not be provided explicitly
dotnet_diagnostic.S3236.severity = none
Copy link
Member

Choose a reason for hiding this comment

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

Why was this one reported? Don't have time to check myself now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It comes when one wants to provide a value for a parameter marked with [CallerExpressionAttribute], e.g.

public TimeSpan Timeout
{
    get
    {
        return _timeout;
    }
    set
    {
        // EnsureValidTimeout(this TimeSpan timeSpan, [CallerArgumentExpression(nameof(timeSpan))] string? paramName = null)

        value.EnsureValidTimeout(nameof(Timeout));

        _timeout = value;
    }
}

I would probably suppress this one at the call sites, but #1480 will also bring more places where we don't want it (from Debug.Assert)

@Rob-Hague
Copy link
Collaborator Author

Thanks

@Rob-Hague Rob-Hague merged commit aac10fb into sshnet:develop Sep 19, 2024
1 check passed
@Rob-Hague Rob-Hague deleted the misc branch September 19, 2024 05:53
@scott-xu
Copy link
Collaborator

Also change around some of the newly added keys - I have a use for this with certificate support.

How about consolidating the keys. e.g. one key-pair per algorithm (and key size) , but with different format and different encryption for private key? It will be easier for integration test.

@Rob-Hague
Copy link
Collaborator Author

It seems OK how it is - it was a bit of a pain to set everything up for #1498 so I would rather like to avoid changing it

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.

3 participants