-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
Update solution to use net 8.0 #1063
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1063 +/- ##
==========================================
- Coverage 74.30% 74.27% -0.03%
==========================================
Files 95 95
Lines 5417 5520 +103
Branches 650 631 -19
==========================================
+ Hits 4025 4100 +75
- Misses 1167 1216 +49
+ Partials 225 204 -21
☔ View full report in Codecov by Sentry. |
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.
can remove a chunk of the #if blocks by using the following workaround. will see if I can get time later this week as traveling
/// <summary>
/// Allows capturing of the expressions passed to a method.
/// Shims for CallerArgumentExpressionAttribute to provide compatability with NET48
/// Taken from: https://weblogs.asp.net/dixin/csharp-10-new-feature-callerargumentexpression-argument-check-and-more on 20230904.
/// </summary>
[AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
internal sealed class CallerArgumentExpressionAttribute : Attribute
{
/// <summary>
/// Initializes a new instance of the <see cref="CallerArgumentExpressionAttribute"/> class.
/// </summary>
/// <param name="parameterName">The name of the targeted parameter.</param>
public CallerArgumentExpressionAttribute(string parameterName) => ParameterName = parameterName;
/// <summary>
/// Gets the target parameter name of the <c>CallerArgumentExpression</c>.
/// </summary>
/// <returns>
/// The name of the targeted parameter of the <c>CallerArgumentExpression</c>.
/// </returns>
public string ParameterName { get; }
}
/// <summary>
/// Shims for ArgumentNullException to provide compatability with NET48.
/// </summary>
internal static class ArgumentNullException
{
/// <summary>Throws an <see cref="T:System.ArgumentNullException" /> if <paramref name="argument" /> is <see langword="null" />.</summary>
/// <param name="argument">The reference type argument to validate as non-null.</param>
/// <param name="paramName">The name of the parameter with which <paramref name="argument" /> corresponds.</param>
/// <exception cref="T:System.ArgumentNullException">
/// <paramref name="argument" /> is <see langword="null" />.</exception>
public static void ThrowIfNull(object? argument, [CallerArgumentExpression("argument")] string? paramName = null)
{
if (argument != null)
{
return;
}
throw new global::System.ArgumentNullException(paramName);
}
}
@dpvreony thank you, I am also travelling in the UK this week, then possibly in India next week or the week after for 2 weeks. |
I guess the disadvantage of the helper method is the inbuilt one gives a clean stack, while this one adds one more layer to the stack call. |
The Api will also have two helpers one per framework difference, in some cases 3 variances, as we will still need to put the #buildFramework elements into the helpers. Personally, I am ok with the #if #else option hence I choose to spend the time to fix them, I don't see us having to change those sections of code again so it's not really a maintenance issue, but which is more performant should be the way we go. |
Stick with the #if for the moment. When netstandard disappears we can strip them out. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
What kind of change does this PR introduce?
Feature - Add Net 8.0
What is the current behavior?
Targets up to net 7.0
What is the new behavior?
Targets up to net 8.0
What might this PR break?
VS2022 Preview will need to be used until the release comes out with net 8.0 support
Please check if the PR fulfills these requirements
Other information: