Skip to content

Update doc and generic parameter name for JsonValue.GetValue - #56639

Merged
steveharter merged 3 commits into
dotnet:mainfrom
steveharter:JsonValueDoc
Aug 3, 2021
Merged

Update doc and generic parameter name for JsonValue.GetValue#56639
steveharter merged 3 commits into
dotnet:mainfrom
steveharter:JsonValueDoc

Conversation

@steveharter

@stevehartersteveharter commented Jul 30, 2021

Copy link
Copy Markdown
Contributor

Adds additional XML doc for the GetValue methods; in particular it wasn't obvious that passing in <object> for the generic parameter will always succeed and return the underlying value.

In addition, the JsonNode.GetValue<TValue> was changed to JsonNode.GetValue<T> to be consistent with JsonValue.TryGetValue<T>. Not sure on the guidance here, but I think consistency is better, and <TValue> seems redundant.

@stevehartersteveharter added documentation Documentation bug or enhancement, does not impact product or test code area-System.Text.Json labels Jul 30, 2021
@stevehartersteveharter added this to the 6.0.0 milestone Jul 30, 2021
@stevehartersteveharter self-assigned this Jul 30, 2021
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Adds additional detail to the GetValue methods; in particular it wasn't obvious that passing in <object> for the generic parameter will always succeed and return the underlying value.

Author:steveharter
Assignees:steveharter
Labels:

area-System.Text.Json, documentation

Milestone:6.0.0

@stevehartersteveharter changed the title Update doc for JsonValue.GetValueUpdate doc and generic parameter name for JsonValue.GetValueJul 30, 2021
public System.Text.Json.Nodes.JsonValue AsValue() { throw null; }
public string GetPath() { throw null; }
public virtual TValue GetValue<TValue>() { throw null; }
public virtual T GetValue<T>() { throw null; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems fine to me?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here. I don't think this needs an API review.

@stevehartersteveharterJul 30, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks (I was about to create a mail for this)

/// The underlying value of a <see cref="JsonValue"/> after deserialization is an instance of <see cref="JsonElement"/>,
/// otherwise it's the value specified when the <see cref="JsonValue"/> was created.
/// </remarks>
/// <seealso cref="JsonNode.GetValue{TValue}"></seealso>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't this also need renaming?

Suggested change
/// <seealso cref="JsonNode.GetValue{TValue}"></seealso>
/// <seealso cref="JsonNode.GetValue{T}"></seealso>

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yep. Thanks

@steveharter
steveharter merged commit 7abe597 into dotnet:mainAug 3, 2021
@steveharter
steveharter deleted the JsonValueDoc branch August 3, 2021 14:18
thaystg added a commit to thaystg/runtime that referenced this pull request Aug 3, 2021
* origin/main: (64 commits)
[wasm][debugger] Create test Inherited Properties (dotnet#56754)
Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
[mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
Update windows-requirements.md (dotnet#56476)
Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
[wasm][debugger] Inspect static class (dotnet#56740)
Fix stack overflow handling issue in GC stress (dotnet#56733)
Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
Move Windows Compat pack to NuGet pack task (dotnet#56686)
Fix build error when building some packages (dotnet#56767)
Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
Fix race in crossdac publishing with PGO (dotnet#56762)
Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
Use ComWrappers in some Marshal unit-tests and update platform metadata (dotnet#56595)
Set `DisableImplicitNamespaceImports_Dotnet=true` to workaround sdk issue (dotnet#56744)
Make sure ServerGCHeapDetails is up to date (dotnet#56056)
[libraries] Reenable System.Diagnostics.DiagnosticSorce.Switches.Tests on mobile (dotnet#56737)
Disable failing arm64 win10 Graphics.FromHdc tests (dotnet#56732)
Match xplat event source conditions (dotnet#56435)
...
thaystg added a commit to thaystg/runtime that referenced this pull request Aug 4, 2021
…ger_proxy_attribute
* origin/main: (340 commits)
add RID for Debian 11 (dotnet#56789)
[wasm] [debugger] Skip thread static field (dotnet#56749)
Fix timeouts in coreroot_determinism test in GC stress mode (dotnet#56770)
Use File.OpenHandle in Socket.SendFile directly (dotnet#56777)
accept empty realm for digest auth (dotnet#56369) (dotnet#56455)
[wasm][debugger] Create test Inherited Properties (dotnet#56754)
Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
[mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
Update windows-requirements.md (dotnet#56476)
Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
[wasm][debugger] Inspect static class (dotnet#56740)
Fix stack overflow handling issue in GC stress (dotnet#56733)
Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
Move Windows Compat pack to NuGet pack task (dotnet#56686)
Fix build error when building some packages (dotnet#56767)
Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
Fix race in crossdac publishing with PGO (dotnet#56762)
Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
Use ComWrappers in some Marshal unit-tests and update platform metadata (dotnet#56595)
...
@ghostghost locked as resolved and limited conversation to collaborators Sep 2, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Text.JsondocumentationDocumentation bug or enhancement, does not impact product or test code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@steveharter@stephentoub@eiriktsarpalis@terrajobst@bartonjs