Skip to content

Merge | TdsParserStateObjectFactory, TdsParserStateObject #3291

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
Merged
merged 4 commits into from
May 9, 2025
Merged

Merge | TdsParserStateObjectFactory, TdsParserStateObject #3291

merged 4 commits into from
May 9, 2025

Conversation

edwardneal
Copy link
Contributor

Fairly small merge here, adjacent to TdsParser. This completely merges TdsParserStateObjectFactory, which should be fairly simple.

More importantly, it also starts to reorganise the netfx class hierarchy to align with netcore. This is the key point I'd like some feedback on. I think we'll need to finish this to make more progress with some of the remaining files.

netfx currently has a single TdsParserStateObject class. In netcore this class is abstract, and all of the native functionality is located in a derived TdsParserStateObjectNative class. To start migrating netfx to this structure, I've:

  • Created a stub TdsParserStateObjectNative class in the netfx project, ensuring it inherits from TdsParserStateObject
  • Marked the original TdsParserStateObject class as abstract, redirected TdsParserStateObjectFactory to instantiate the Native derived class
  • Started to move the abstract method definitions in TdsParserStateObject's netcore definition to netfx
  • Where the netcore project's TdsParserStateObjectNative implementation of these abstract method definitions matches netfx, merge the two implementations

I've started small, with five methods which just pass through to already-shared code and don't maintain state.

I'd appreciate a CI run please.

@cheenamalhotra
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov Codecov
Copy link

codecov bot commented Apr 23, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.

Project coverage is 62.20%. Comparing base (14bee54) to head (30e3250).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 80.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3291      +/-   ##
==========================================
- Coverage   65.07%   62.20%   -2.88%     
==========================================
  Files         298      293       -5     
  Lines       65515    65254     -261     
==========================================
- Hits        42634    40588    -2046     
- Misses      22881    24666    +1785
Flag Coverage Δ
addons ?
netcore 66.62% <100.00%> (-1.72%) ⬇️
netfx 60.65% <92.00%> (-5.52%) ⬇️

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.

benrr101
benrr101 approved these changes Apr 24, 2025
View reviewed changes
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

Overall, I think this is a good step! I'm curious about your larger scope ideas and how we can combine our efforts to get merging the projects wrapped up. The team is interested in chatting more with you about it, if you're open to it, feel free to shoot me an email at the email on my profile (since you're pretty secretive, haha)

@edwardneal
Copy link
Contributor Author

Thanks, I've just emailed.

The biggest hurdle for the merge is TdsParser, and the most complex part of that is instantiating and calling into TdsParserStateObject. I'm trying to work out a path towards merging that by dealing with the instantiation, then proving the concept of swapping out the simplest virtual method calls. Accessing variables becomes a little more complex as we merge parts of TdsParserStateObjectNative, I'm not sure whether to merge every method which touches the same set of variables or to just change their scope to protected in netfx's TdsParserStateObject definition - there'll be some testing needed.

@benrr101 benrr101 added this to the 6.1-preview2 milestone Apr 29, 2025
@benrr101
Copy link
Contributor

benrr101 commented May 1, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@edwardneal edwardneal mentioned this pull request May 1, 2025
Open
@benrr101
Copy link
Contributor

benrr101 commented May 8, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@benrr101 benrr101 merged commit e796d85 into dotnet:main May 9, 2025
237 checks passed
@edwardneal edwardneal deleted the merge/tdsparserstateobjectfactory branch May 9, 2025 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benrr101 benrr101 benrr101 approved these changes

@paulmedynski paulmedynski paulmedynski approved these changes

@samsharma2700 samsharma2700 samsharma2700 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.1-preview2
Development

Successfully merging this pull request may close these issues.
5 participants
@edwardneal @cheenamalhotra @benrr101 @paulmedynski @samsharma2700