-
Notifications
You must be signed in to change notification settings - Fork 693
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
Remove PackageDependencyGroupConverter #5872
Changes from 16 commits
b628223
5299ace
f050e96
92a5e08
1cd48e3
5a2349d
9556342
7210a2e
edb5f6e
7856ac6
64bb8b3
9e99c32
9fa0425
fbfce3e
f14e6f7
78918c9
ce23ff7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using Newtonsoft.Json; | ||
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Unit Tests on Linux (.NET 8.0) Unit Tests on Linux (.NET 8.0))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Unit Tests on Linux (.NET 8.0) Unit Tests on Linux (.NET 8.0))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Unit Tests on MacOS (.NET Core 3.1) Unit Tests on MacOS (.NET Core 3.1))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Unit Tests on MacOS (.NET Core 3.1) Unit Tests on MacOS (.NET Core 3.1))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Functional Tests on Linux (.NET Core 3.1) Functional Tests on Linux (.NET Core 3.1))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Functional Tests on Linux (.NET Core 3.1) Functional Tests on Linux (.NET Core 3.1))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Unit Tests on Linux (.NET Core 3.1) Unit Tests on Linux (.NET Core 3.1))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Unit Tests on Linux (.NET Core 3.1) Unit Tests on Linux (.NET Core 3.1))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Unit Tests on MacOS (.NET 8.0) Unit Tests on MacOS (.NET 8.0))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Unit Tests on MacOS (.NET 8.0) Unit Tests on MacOS (.NET 8.0))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Functional Tests on Linux (.NET 8.0) Functional Tests on Linux (.NET 8.0))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Functional Tests on Linux (.NET 8.0) Functional Tests on Linux (.NET 8.0))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Functional Tests on MacOS (.NET 8.0) Functional Tests on MacOS (.NET 8.0))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Functional Tests on MacOS (.NET 8.0) Functional Tests on MacOS (.NET 8.0))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Functional Tests on MacOS (.NET Core 3.1) Functional Tests on MacOS (.NET Core 3.1))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
Check failure on line 5 in src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Azure Pipelines / NuGet.Client CI (Functional Tests on MacOS (.NET Core 3.1) Functional Tests on MacOS (.NET Core 3.1))src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs#L5
|
||
using NuGet.Frameworks; | ||
|
||
namespace NuGet.Protocol.Converters | ||
{ | ||
internal class NuGetFrameworkConverter : JsonConverter | ||
martinrrm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer) | ||
{ | ||
serializer.Serialize(writer, value.ToString()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: this is... complicated 😆 The TFM value most people are used to reading, is not the value returned by But since this is a generic I wonder if we can use this overload of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like your comment but since this is targeted for 17.2 P3 I believe I don't have enough time for this at this time, do you want me to investigate this further for next release? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly, I think my preference would be to throw on write, like the old converter did. If anything needs to convert a NuGetFramework to a string in the future, then we can either investigate that "parameterized" converter, or create one converter per string format. But I can also live with using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we throw a not implemented exception here if there are going to be no callers for this method? We can address Andy's feedback in a follow up PR. The existing code throw not implemented exception for the |
||
} | ||
|
||
public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer) | ||
{ | ||
var value = reader.Value.ToString(); | ||
var framework = NuGetFramework.AnyFramework; | ||
|
||
if (!string.IsNullOrEmpty(value)) | ||
{ | ||
framework = NuGetFramework.Parse(value); | ||
} | ||
|
||
return framework; | ||
} | ||
|
||
public override bool CanConvert(Type objectType) => objectType == typeof(NuGetFramework); | ||
} | ||
} |
This file was deleted.
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.
I don't understand how the JSON deserialization sets the packages list, if this is the json constructor. But there are tests that pass? 😕
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's weird, since there is already a constructor that takes
targetFramework
and dependencies and null checks them I didn't want to change the behavior of it.Previous converter would initialize an empty list for dependencies when it was null (no dependencies property in the json) thats why I added a constructor that takes only the
TargetFramework
that helps me create the empty list, this constructor is called only when deserialization happens. If we add a [JsonConstructor] it first attempts to use it and then populate the rest of the properties that have [JsonProperty].JsonProperty
doesn't needs a constructor to work.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.
The properties are get only, and they're not even auto-props. If Newtonsoft.Json is using reflection to set the value directly on the backing store, I don't understand how it can determine what the backing field name is. 🤷