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.

Sign up for GitHub

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

Jump to bottom

Add a JsonObject.TryAdd method. #110244

Open
eiriktsarpalis opened this issue Nov 28, 2024 · 4 comments · May be fixed by #111229
Open

Add a JsonObject.TryAdd method. #110244

eiriktsarpalis opened this issue Nov 28, 2024 · 4 comments · May be fixed by #111229
Assignees
@Flu
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json in-pr There is an active PR which will close this issue when it is merged
Milestone
10.0.0

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Nov 28, 2024 •
edited
Loading

This workaround requires dual lookup if the key is missing, which suggests to me that a JsonObject.TryAddmethod similar to the one in #107947 might be necessary.

Originally posted by @eiriktsarpalis in dotnet/aspire#6312 (comment)

API Proposal

Exposing the APIs as approved for OrderedDictionary to JsonObject:

namespace System.Text.Json.Nodes;

public partial class JsonObject
{
    public bool TryAdd(string propertyName, JsonNode? value, out int index);
    public bool TryGetPropertyValue(string propertyName, out JsonNode? jsonNode, out int index);
}
@eiriktsarpalis eiriktsarpalis transferred this issue from dotnet/aspire Nov 28, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 28, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Nov 28, 2024
@eiriktsarpalis eiriktsarpalis added area-System.Text.Json and removed untriaged New issue has not been triaged by the area owner needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 28, 2024
@eiriktsarpalis eiriktsarpalis changed the title This workaround requires dual lookup if the key is missing, which suggests to me that a JsonObject.TryAddmethod similar to the one in https://github.com/dotnet/runtime/issues/107947 might be necessary. Add a JsonObject.TryAdd method. Nov 28, 2024
@eiriktsarpalis eiriktsarpalis added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 28, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

@eiriktsarpalis eiriktsarpalis self-assigned this Nov 28, 2024
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 28, 2024
@terrajobst
Copy link
Member

terrajobst commented Dec 3, 2024 •
edited
Loading

Video

  • Looks good as proposed but we should add an overload of TryAdd without the index for consistency and ergonomics
  • The behavior of what we return for the index should match OrderedDictionary<TKey, TValue>
namespace System.Text.Json.Nodes;

public partial class JsonObject
{
    public bool TryAdd(string propertyName, JsonNode? value);
    public bool TryAdd(string propertyName, JsonNode? value, out int index);
    public bool TryGetPropertyValue(string propertyName, out JsonNode? jsonNode, out int index);
}

@terrajobst terrajobst added the api-approved API was approved in API review, it can be implemented label Dec 3, 2024
@eiriktsarpalis eiriktsarpalis added this to the 10.0.0 milestone Dec 3, 2024
@terrajobst terrajobst removed the api-ready-for-review API is ready for review, it is NOT ready for implementation label Dec 6, 2024
@Flu
Copy link

Flu commented Jan 8, 2025

Is there anybody working on this? If not, I can take a stab at it.

@eiriktsarpalis
Copy link
Member Author

@Flu go for it, thanks!

@eiriktsarpalis eiriktsarpalis assigned Flu and unassigned eiriktsarpalis Jan 8, 2025
@Flu Flu linked a pull request Jan 9, 2025 that will close this issue
Draft
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Flu Flu

Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Milestone
10.0.0
Development

Successfully merging a pull request may close this issue.

Add Try methods to JsonObject Flu/runtime
3 participants
@eiriktsarpalis @terrajobst @Flu