-
-
Notifications
You must be signed in to change notification settings - Fork 934
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
Conversation
# Conflicts: # src/Renci.SshNet/Renci.SshNet.csproj
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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
Thanks |
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. |
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 |
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.