Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add ToString() to SqlJson#3427

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
cheenamalhotra merged 6 commits intomainfromdev/paul/json-tostring
Jun 25, 2025
Merged

Conversation

paulmedynski
Copy link
Contributor

@paulmedynskipaulmedynski commentedJun 17, 2025•
edited
Loading

Description

  • Added a ToString() method to SqlJson that returns the serialized JSON string or null.
  • Added StringSyntax JSON to the string constructor's argument.
  • Updated XML documentation to include descriptive summaries, parameters, and exceptions.
  • Simplified the SqlJson implementation.
  • Moved existing tests to the UnitTests project and improved them.

Issues

Addresses#3424 and#3107 .

Testing

Unit tests were run manually, and CI will cover regressions.

- Added ToString() to SqlJson.- Added missing commentary to public API docs.- Simplified implementation.- Improved tests and moved them to the UnitTests project.
@CopilotCopilotAI review requested due to automatic review settingsJune 17, 2025 18:21
@paulmedynskipaulmedynski requested a review froma team as acode ownerJune 17, 2025 18:21
Copy link
Contributor

@CopilotCopilotAI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds aToString() override toSqlJson, refactors constructors to validate JSON viaJsonDocument.Parse, migrates manual tests into unit tests, and updates XML documentation.

  • ImplementToString() to return the underlying JSON string or null.
  • Simplify null-handling logic in constructors and validate inputs withJsonDocument.Parse.
  • Replace manual test file with comprehensive unit tests and update documentation snippets.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
FileDescription
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlTypes/SqlJson.csAddedToString(), removed legacy null field, streamlined constructors
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlTypes/SqlJsonTest.csNew unit tests covering constructors,Value, andToString()
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csprojRemoved manual test entry forSqlJsonTest
src/Microsoft.Data.SqlClient/tests/ManualTests/Json/SqlJsonTest.csDeleted legacy manual test file
doc/snippets/Microsoft.Data.SqlTypes/SqlJson.xmlUpdated XML docs for constructors, added<ToString> docs
Comments suppressed due to low confidence (1)

doc/snippets/Microsoft.Data.SqlTypes/SqlJson.xml:63

  • [nitpick] Consider adding a<returns> element under<ToString> to explicitly describe the return value (including its null semantics).
    <ToString>

@paulmedynskipaulmedynski added Public API 🆕Issues/PRs that introduce new APIs to the driver. Area\JsonUse this for issues that are targeted for the Json feature in the driver. P2Use to label moderate priority issue - impacts atleast more than 1 customer. labelsJun 17, 2025
@paulmedynskipaulmedynski added this to the6.1-preview2 milestoneJun 17, 2025
@paulmedynskipaulmedynski linked an issueJun 17, 2025 that may beclosed by this pull request
Copy link
ContributorAuthor

@paulmedynskipaulmedynski left a comment

Choose a reason for hiding this comment

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

Added commentary for reviewers.

Copy link
Contributor

@apoorvdeshmukhapoorvdeshmukh left a comment

Choose a reason for hiding this comment

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

Added some minor comments.

cheenamalhotra
cheenamalhotra previously approved these changesJun 18, 2025
Copy link
Member

@cheenamalhotracheenamalhotra left a comment•
edited
Loading

Choose a reason for hiding this comment

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

It seems integration (manual) tests using SqlDataReader to test SqlJson are not running, which should be running now against Azure for a more comprehensive code coverage around Json feature.

Since its out of scope for this PR, can you create an issue and we can try assigning to Copilot to configure the same (after PR#3429 is merged) - and we should be able to run those tests against Azure SQL DBs now.

@codecovCodecov
Copy link

codecovbot commentedJun 18, 2025•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.83%. Comparing base(78dec95) to head(c0791a8).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@##             main    #3427      +/-   ##==========================================+ Coverage   63.59%   69.83%   +6.23%==========================================  Files         299      280      -19       Lines       65418    62169    -3249     ==========================================+ Hits        41601    43414    +1813+ Misses      23817    18755    -5062
FlagCoverage Δ
addons92.58% <ø> (ø)
netcore72.92% <100.00%> (+6.04%)⬆️
netfx69.33% <100.00%> (+3.30%)⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

🚀 New features to boost your workflow:
  • ❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@paulmedynskipaulmedynski linked an issueJun 18, 2025 that may beclosed by this pull request
- Added StringSyntax JSON attribute to the string constructor.- Disposing of JsonDocument used for validation.- Enabled CA2000 analyzer as a suggestion.
Copy link
ContributorAuthor

@paulmedynskipaulmedynski left a comment

Choose a reason for hiding this comment

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

Move commentary for reviewers.

Copy link
Contributor

@benrr101benrr101 left a comment

Choose a reason for hiding this comment

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

Mostly just looking for changes to the tests.

benrr101
benrr101 previously approved these changesJun 19, 2025
Copy link
Contributor

@benrr101benrr101 left a comment

Choose a reason for hiding this comment

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

A few nits, but I won't hold it up on this. My tolerance level for test code is a lot higher than prod code.

Copy link

@campersaucampersau left a comment

Choose a reason for hiding this comment

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

Test using nits.

Copy link
Member

@cheenamalhotracheenamalhotra left a comment

Choose a reason for hiding this comment

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

Rest everything looks good!

- Added ToString() to SqlJson in the ref projects.
@cheenamalhotracheenamalhotra merged commit195de03 intomainJun 25, 2025
251 checks passed
@cheenamalhotracheenamalhotra deleted the dev/paul/json-tostring branchJune 25, 2025 03:35
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@campersaucampersaucampersau left review comments

@edwardnealedwardnealedwardneal left review comments

Copilot code reviewCopilotCopilot left review comments

@benrr101benrr101benrr101 approved these changes

@cheenamalhotracheenamalhotracheenamalhotra approved these changes

@apoorvdeshmukhapoorvdeshmukhAwaiting requested review from apoorvdeshmukh

Assignees
No one assigned
Labels
Area\JsonUse this for issues that are targeted for the Json feature in the driver.P2Use to label moderate priority issue - impacts atleast more than 1 customer.Public API 🆕Issues/PRs that introduce new APIs to the driver.
Projects
None yet
Milestone
6.1-preview2
Development

Successfully merging this pull request may close these issues.

Microsoft.DataSqlTypes.SqlJson is missing ToString [API Proposal] Add StringSyntaxAttributes to SqlJson
6 participants
@paulmedynski@benrr101@campersau@apoorvdeshmukh@cheenamalhotra@edwardneal
[8]ページ先頭
©2009-2025 Movatter.jp