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

Add missing fields that make usage fail #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Falco20019
Copy link
Collaborator

@Falco20019 Falco20019 commented Aug 6, 2024

I tried to use distroessed with .NET 6 and it failed due to not mapping vs-mac-version and vs-mac-support.

@@ -78,6 +78,10 @@ public record RuntimeComponent(
JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm also not sure if the type for those with JsonIgnore are correct. I would assume that these should be string? instead.

@richlander
Copy link
Owner

Re-using some earlier feedback ...

https://github.com/richlander/distroessed/pull/8/files/1ebe54fe77ef8d814043bb1952a57ca721273442#diff-4757b05d5af996f8549bad46428706e5f8109f41d4fef6a20a861e72192b2f2d

I'd prefer to not use [JsonUnmappedMemberHandling(JsonUnmappedMemberHandling.Disallow)] for objects with these properties. This isn't ideal, but it seems much better than the alternative.

This PR is still open: dotnet/core#9425. It's not currently a priority to resolve that, but is very much where we're headed. I want to ensure that these properties don't get written to files like https://github.com/dotnet/core/blob/main/release-notes/8.0/8.0.0/release.json. That may happen if we accept this change.

There are other properties like this, like https://github.com/dotnet/core/blob/2d75c33267d6e37bcbe91b313237a74961ef5774/release-notes/1.0/releases.json#L214. In the tool where the release.json files are generated, I'm choosing to ignore all those old properties. I'm using the most naive approach (for convenience) for reading those files, so the old releases.json files are relevant. They are used to populate releases-index.json.

@richlander
Copy link
Owner

richlander commented Aug 30, 2024

Sorry for taking so long to give feedback on these PRs! I definitely want to get the PRs merged.

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.

2 participants