NotificationsYou must be signed in to change notification settings
Fork311
Star927

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.

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

Jump to bottom

Merged

benrr101 merged 4 commits intodotnet:mainfromedwardneal:merge/tdsparserstateobjectfactory

May 9, 2025

Merged

Merge | TdsParserStateObjectFactory, TdsParserStateObject#3291

benrr101 merged 4 commits intodotnet:mainfromedwardneal:merge/tdsparserstateobjectfactory

May 9, 2025

Conversation

Copy link

Contributor

edwardneal commentedApr 22, 2025

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.

edwardneal added2 commits

April 22, 2025 21:24

Merge TdsParserStateObjectFactory

39b1167

Port one-line methods to netfx TdsParserStateObjectNative

d7e1bf8

Copy link

Member

cheenamalhotra commentedApr 23, 2025

/azp run

Copy link

azure-pipelinesbot commentedApr 23, 2025

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

codecovbot commentedApr 23, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is93.10345% with2 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 approved these changes

Apr 24, 2025

View reviewed changes

Copy link

Contributor

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)

Copy link

ContributorAuthor

edwardneal commentedApr 25, 2025

Thanks, I've just emailed.

The biggest hurdle for the merge is TdsParser, and the most complex part of that is instantiating and calling intoTdsParserStateObject. 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 ofTdsParserStateObjectNative, I'm not sure whether to merge every method which touches the same set of variables or to just change their scope toprotected in netfx'sTdsParserStateObject definition - there'll be some testing needed.