-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Convert.ToHexStringLower
: lower variant for Convert.ToHexString
#92483
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
@dotnet-policy-service agree |
It'd be good to include in this PR updates to places elsewhere in the repo we can switch to using the new public API, e.g. Line 233 in 0841678
|
Nevermind I see that is addressed here:
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsNew methods: namespace System
{
public static class Convert
{
public static string ToHexStringLower(byte[] inArray);
public static string ToHexStringLower(byte[] inArray, int offset, int length);
public static string ToHexStringLower(ReadOnlySpan<byte> bytes);
}
} Close #60393.
|
@determ1ne could you apply the feedback? In addition to above case, I see 5 hits in: runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs Line 1785 in 52bd95c
Line 200 in 52bd95c
Otherwise, the PR looks good, thank you! |
@stephentoub, @buyaa-n I tried to patch all |
Thanks, looks good to me, you have got a conflict in
This should be contributing to #60393 not closing, should be closed when |
a221e20
to
73ee107
Compare
@buyaa-n I've implemented |
src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs
Show resolved
Hide resolved
...libraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/Convert.ToHexString.cs
Show resolved
Hide resolved
`System.Diagnostics.DiagnosticSource`
dca0505
to
660890f
Compare
#if NET5_0_OR_GREATER | ||
internal static string EncodeHexString(byte[] sArray) | ||
{ | ||
return Convert.ToHexString(sArray); | ||
} | ||
#else | ||
internal static string EncodeHexString(byte[] sArray) | ||
{ | ||
return HexConverter.ToString(sArray); | ||
} | ||
#endif |
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.
This doesn't seem need an if-def, it was added same time as Convert.ToHexString(...)
added, was it failing in other builds?
#if NET5_0_OR_GREATER | |
internal static string EncodeHexString(byte[] sArray) | |
{ | |
return Convert.ToHexString(sArray); | |
} | |
#else | |
internal static string EncodeHexString(byte[] sArray) | |
{ | |
return HexConverter.ToString(sArray); | |
} | |
#endif | |
internal static string EncodeHexString(byte[] sArray) | |
{ | |
return Convert.ToHexString(sArray); | |
} |
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.
From nuget the System.Security.Cryptography.Xml 8.0.0
package is still targeting .net framework, while Convert.ToHexString
is added in net5.0, so I'm not sure if it will build (or works).
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.
Please check the CI result https://github.com/dotnet/runtime/pull/92483/checks?check_run_id=18807551051 , removing the if-def will fail the build.
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.
Thanks for checking, the file doesn't included in .netframework build, probably failing on netstandard
src/libraries/System.Security.Cryptography.Pkcs/src/Internal/Cryptography/PkcsHelpers.cs
Show resolved
Hide resolved
...s/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/SignedXmlDebugLog.cs
Show resolved
Hide resolved
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.
LGMT, thank you @determ1ne ,
The failure unrelated and known.
New methods:
Close #60393.
TryToHexStringLower
may be implemented after #86556 merged.