-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 Try methods to JsonObject #111229
base: main
Are you sure you want to change the base?
Add Try methods to JsonObject #111229
Changes from all commits
2a3106b
2252001
76c0f51
6121c38
3ded485
1672a5b
1fa1a6d
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 | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -34,6 +34,47 @@ public void Add(string propertyName, JsonNode? value) | |||||||||
value?.AssignParent(this); | ||||||||||
} | ||||||||||
|
||||||||||
/// <summary> | ||||||||||
/// Adds an element with the provided name and value to the <see cref="JsonObject"/>, if a property named <paramref name="propertyName"/> doesn't already exist. | ||||||||||
/// </summary> | ||||||||||
/// <param name="propertyName">The property name of the element to add.</param> | ||||||||||
/// <param name="value">The value of the element to add.</param> | ||||||||||
/// <exception cref="ArgumentNullException"><paramref name="propertyName"/> is null.</exception> | ||||||||||
/// <returns> | ||||||||||
/// <see langword="true"/> if the property didn't exist and the element was added; otherwise, <see langword="false"/>. | ||||||||||
/// </returns> | ||||||||||
public bool TryAdd(string propertyName, JsonNode? value) => TryAdd(propertyName, value, out _); | ||||||||||
|
||||||||||
/// <summary> | ||||||||||
/// Adds an element with the provided name and value to the <see cref="JsonObject"/>, if a property named <paramref name="propertyName"/> doesn't already exist. | ||||||||||
/// </summary> | ||||||||||
/// <param name="propertyName">The property name of the element to add.</param> | ||||||||||
/// <param name="value">The value of the element to add.</param> | ||||||||||
/// <param name="index">The index of the added or existing <paramref name="propertyName"/>. This is always a valid index into the <see cref="JsonObject"/>.</param> | ||||||||||
/// <exception cref="ArgumentNullException"><paramref name="propertyName"/> is null.</exception> | ||||||||||
/// <returns> | ||||||||||
/// <see langword="true"/> if the property didn't exist and the element was added; otherwise, <see langword="false"/>. | ||||||||||
/// </returns> | ||||||||||
public bool TryAdd(string propertyName, JsonNode? value, out int index) | ||||||||||
{ | ||||||||||
if (propertyName is null) | ||||||||||
{ | ||||||||||
ThrowHelper.ThrowArgumentNullException(nameof(propertyName)); | ||||||||||
} | ||||||||||
|
||||||||||
#if NET10_0_OR_GREATER | ||||||||||
var success = Dictionary.TryAdd(propertyName, value, out index); | ||||||||||
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. This is probably failing to build in .NET 9 since that version of OrderedDictionary doesn't have the overload that exposes the index. I would recommend emulating the behavior using a second lookup via |
||||||||||
#else | ||||||||||
var success = Dictionary.TryAdd(propertyName, value); | ||||||||||
index = Dictionary.IndexOf(propertyName); | ||||||||||
Comment on lines
+68
to
+69
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.
Suggested change
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. Shouldn't this be Count - 1? Also, this assumes that OD appends new entries at the end, but what if that changes in the future? 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. Yes, it should be 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. Yes that should've been
Hmm, independent of however it's currently implemented, what invariant is An alternative is to do Not optimizing this case is also fine. 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. To be clear, I don't think that it would ever change; I'm just pointing out that it's depending on an invariant from a separate component.
It's a dictionary that also implements a deterministic IList with 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. So what is the consensus here? Do I leave it as it is? 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'm fine to apply the optimisation provided there is test coverage, e.g. a test that verifies that the returned index is correct in that scenario. 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. A comment above the line explaining the arrangement might help as well. |
||||||||||
#endif | ||||||||||
if (success) | ||||||||||
{ | ||||||||||
value?.AssignParent(this); | ||||||||||
} | ||||||||||
return success; | ||||||||||
} | ||||||||||
|
||||||||||
/// <summary> | ||||||||||
/// Adds the specified property to the <see cref="JsonObject"/>. | ||||||||||
/// </summary> | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,17 +115,42 @@ internal string GetPropertyName(JsonNode? node) | |
/// </summary> | ||
/// <param name="propertyName">The name of the property to return.</param> | ||
/// <param name="jsonNode">The JSON value of the property with the specified name.</param> | ||
/// <exception cref="ArgumentNullException"> | ||
/// <paramref name="propertyName"/> is <see langword="null"/>. | ||
/// </exception> | ||
/// <returns> | ||
/// <see langword="true"/> if a property with the specified name was found; otherwise, <see langword="false"/>. | ||
/// </returns> | ||
public bool TryGetPropertyValue(string propertyName, out JsonNode? jsonNode) | ||
public bool TryGetPropertyValue(string propertyName, out JsonNode? jsonNode) => TryGetPropertyValue(propertyName, out jsonNode, out _); | ||
|
||
/// <summary> | ||
/// Gets the value associated with the specified property name. | ||
/// </summary> | ||
/// <param name="propertyName">The property name of the value to get.</param> | ||
/// <param name="jsonNode"> | ||
/// When this method returns, it contains the value associated with the specified property name, if the property name is found; | ||
/// otherwise <see langword="null"/>. | ||
/// </param> | ||
/// <param name="index">The index of <paramref name="propertyName"/> if found; otherwise, -1.</param> | ||
/// <exception cref="ArgumentNullException"> | ||
/// <paramref name="propertyName"/> is <see langword="null"/>. | ||
/// </exception> | ||
/// <returns> | ||
/// <see langword="true"/> if the <see cref="JsonObject"/> contains an element with the specified property name; otherwise, <see langword="false"/>. | ||
/// </returns> | ||
public bool TryGetPropertyValue(string propertyName, out JsonNode? jsonNode, out int index) | ||
{ | ||
if (propertyName is null) | ||
{ | ||
ThrowHelper.ThrowArgumentNullException(nameof(propertyName)); | ||
} | ||
|
||
#if NET10_0_OR_GREATER | ||
return Dictionary.TryGetValue(propertyName, out jsonNode, out index); | ||
#else | ||
index = Dictionary.IndexOf(propertyName); | ||
return Dictionary.TryGetValue(propertyName, out jsonNode); | ||
Comment on lines
+151
to
152
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. Similar to above, the 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. Yes, this one can be improved: index = Dictionary.IndexOf(propertyName);
if (index == -1)
{
return Dictionary.TryGetValue(propertyName, out jsonNode);
}
jsonNode = null;
return false; Something like this for the .NET 9 part 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. Once you have the index you don't need to perform another key-based lookup: index = Dictionary.IndexOf(propertyName);
if (index < 0)
{
jsonNode = null;
return false;
}
jsonNode = Dictionary.GetAt(index);
return true; |
||
#endif | ||
} | ||
|
||
/// <inheritdoc/> | ||
|
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.
We only need this for .NET 9. Earlier targets polyfill the current OrderedDictionary implementation.
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 assume it becomes this then?
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.
Right.