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

Update solution to use net 8.0 #1063

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Update solution to use net 8.0 #1063

merged 1 commit into from
Nov 14, 2023

Conversation

ChrisPulman
Copy link
Member

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

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:

Copy link

codecov bot commented Nov 11, 2023

Codecov Report

Attention: 58 lines in your changes are missing coverage. Please review.

Comparison is base (4d7b838) 74.30% compared to head (55c0983) 74.27%.

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     
Files Coverage Δ
....AppCenter/AppCenterFeatureUsageTrackingSession.cs 85.18% <ø> (ø)
src/Splat.Avalonia.DryIoc/AvaloniaMixins.cs 87.69% <100.00%> (+16.78%) ⬆️
src/Splat.Drawing/Bitmaps/BitmapLoaderException.cs 0.00% <ø> (ø)
src/Splat.DryIoc/DryIocDependencyResolver.cs 90.54% <100.00%> (+1.26%) ⬆️
...rc/Splat.Exceptionless/ExceptionlessSplatLogger.cs 84.00% <100.00%> (+9.45%) ⬆️
...DependencyInjection/MicrosoftDependencyResolver.cs 71.61% <100.00%> (+1.10%) ⬆️
...nsions.Logging/MicrosoftExtensionsLoggingLogger.cs 95.89% <100.00%> (+6.65%) ⬆️
.../Splat.SimpleInjector/SimpleInjectorInitializer.cs 58.53% <100.00%> (+1.03%) ⬆️
...pleInjector/TransientSimpleInjectorRegistration.cs 100.00% <100.00%> (ø)
...Monitoring/EnableFeatureUsageTrackingExtensions.cs 51.72% <100.00%> (+5.57%) ⬆️
... and 19 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dpvreony dpvreony left a 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);
        }
    }

@ChrisPulman
Copy link
Member Author

@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.

@glennawatson
Copy link
Contributor

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.

@ChrisPulman
Copy link
Member Author

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.

@glennawatson
Copy link
Contributor

Stick with the #if for the moment.

When netstandard disappears we can strip them out.

@ChrisPulman ChrisPulman merged commit 42980da into main Nov 14, 2023
1 of 3 checks passed
@ChrisPulman ChrisPulman deleted the CP_AddNet8_0 branch November 14, 2023 01:06
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants