Skip to content

Conversation

@dougbu
Copy link
Contributor

@dougbu dougbu commented Nov 12, 2020

  • $(...PackageVersion) -> $(...Version)
  • inspired by a similar dotnet/efcore change
  • aligns names with Arcade SDK, meaning our values apply consistently

Remove unused PublishSymbols.proj and related property

  • restore Arcade's control of $(MicrosoftSymbolUploaderBuildTaskVersion>)
  • would otherwise result in a version downgrade

Update $(MicrosoftNetCompilersToolsetVersion) to match Arcade's value

  • avoid using an older toolset
  • now successfully overriding Arcade value

@dougbu dougbu requested a review from a team November 12, 2020 19:51
@dougbu
Copy link
Contributor Author

dougbu commented Nov 12, 2020

FYI this big change is why I temporarily disabled our Maestro++ subscriptions for the master branch. Will enable them again once this is in i.e. after we can avoid massive conflicts.

Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

Looks good assuming there's nothing trickier than what you called out in the description

@dougbu
Copy link
Contributor Author

dougbu commented Nov 12, 2020

Quick note on what still spells out "packageversion": The main thing is of course the $(PackageVersion) property. I also left properties from the .NET SDK or Arcade alone e.g. $(BundledNETCoreAppPackageVersion) and $(StabilizePackageVersion). But the biggest area is our $(BaselinePackageVersion) properties.

@dougbu
Copy link
Contributor Author

dougbu commented Nov 12, 2020

Help please @JamesNK and @pranavkm. I'm confused by the error CS8603: Possible null reference return. about src/SignalR/common/Shared/JsonUtils.cs, specifically the return defaultValue line in

public static T GetOptionalProperty<T>(JObject json, string property, JTokenType expectedType = JTokenType.None, T defaultValue = default)
{
    var prop = json[property];

    if (prop == null)
    {
        return defaultValue;
    }

    return GetValue<T>(property, expectedType, prop);
}

As I read this, defaultValue should already be of the T type i.e. if default is null then T is Nullable<Something> or Something?. What am I missing❔


VS Code (though not the CI build) also reports the same error later in the file:

public static string? ReadAsString(JsonTextReader reader, string propertyName)
{
    reader.Read();

    if (reader.TokenType != JsonToken.String)
    {
        throw new InvalidDataException($"Expected '{propertyName}' to be of type {JTokenType.String}.");
    }

    return reader.Value?.ToString();
}

This looks even stranger to me because the return is nullable.

@dougbu
Copy link
Contributor Author

dougbu commented Nov 12, 2020

I recall seeing the above error when building this branch locally but it wasn't nearly as consistent. New warnings or errors are likely occurring because we're now using our configured version of Microsoft.CodeAnalysis.CSharp and not Arcade's older choice. But, as I said, I don't "get" that particular CS8603.

@BrennanConroy
Copy link
Member

BrennanConroy commented Nov 12, 2020

You can delete the GetOptionalProperty function. It isn't used anywhere and is internal.

I wonder if a value type for T would cause that? Nope, those would have a non-null default value

@dougbu
Copy link
Contributor Author

dougbu commented Nov 12, 2020

Thanks @BrennanConroy the pipeline is running much smoother now 😺

- `$(...PackageVersion)` -> `$(...Version)`
- inspired by a similar dotnet/efcore change
- aligns names with Arcade SDK, meaning our values apply consistently
- restore Arcade's control of `$(MicrosoftSymbolUploaderBuildTaskVersion>)`
- would otherwise result in a version downgrade
- avoid using an older toolset
- now successfully overriding Arcade value
- delete the unused `GetOptionalProperty(...)` method
- `%(ReferencePathWithRefAssemblies.NuGetPackageVersion)` is the correct name
@dougbu dougbu force-pushed the dougbu/version.properties branch from 8eff07c to 61c894d Compare November 13, 2020 02:12
@dougbu dougbu merged commit 44c0e66 into master Nov 13, 2020
@dougbu dougbu deleted the dougbu/version.properties branch November 13, 2020 18:52
dougbu added a commit to jkoritzinsky/aspnetcore that referenced this pull request Nov 16, 2020
- adjust version properties to align with 44c0e66 / dotnet#27784
dougbu added a commit that referenced this pull request Nov 17, 2020
…th other non-shipping packages from dotnet/runtime to get a non-shipping version. (#27737)

* Remove usage of the Microsoft.NETCore.Internal package and replace with other non-shipping packages from dotnet/runtime to get a non-shipping version.

* Use BrowserDebugHost as the only sentinel package.

* Apply suggestions from code review
- adjust version properties to align with 44c0e66 / #27784

Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
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.

4 participants