Merge more methods from TdsParserStateObject

[bhugot]

more merge after #1520 It would maybe help to start working on fix the async bug.
I think we should be able to do more.

[codecov]

Codecov Report

Base: 69.19% // Head: 70.21% // Increases project coverage by +1.01% 🎉

Coverage data is based on head (639b130) compared to base (6511535).
Patch coverage: 83.35% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1843      +/-   ##
==========================================
+ Coverage   69.19%   70.21%   +1.01%     
==========================================
  Files         292      292              
  Lines       61727    61399     -328     
==========================================
+ Hits        42713    43109     +396     
+ Misses      19014    18290     -724     

Flag	Coverage Δ
addons	92.38% <ø> (ø)
netcore	74.16% <81.12%> (+0.86%)	⬆️
netfx	68.47% <82.48%> (+0.89%)	⬆️

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

Impacted Files	Coverage Δ
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	24.65% <0.00%> (-1.54%)	⬇️
...c/Microsoft/Data/SqlClient/TdsParserSessionPool.cs	86.25% <ø> (ø)
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	17.11% <10.52%> (-6.19%)	⬇️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	85.68% <86.45%> (-3.43%)	⬇️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs	70.84% <95.23%> (+0.01%)	⬆️
...etfx/src/Microsoft/Data/SqlClient/SqlDataReader.cs	68.46% <100.00%> (+0.29%)	⬆️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs	67.71% <100.00%> (ø)
...re/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs	68.85% <0.00%> (-4.92%)	⬇️
.../Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs	76.72% <0.00%> (-0.63%)	⬇️
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[JRahnama]

/Azurepipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[panoskj]

@bhugot @JRahnama as I hinted in #1520 (comment) I have also merged about 2000 more lines of this file. I was actually about to make a PR about it right now.

I can't review this at the moment, but at a glance it looks like you have made changes to more files than I did, but I have merged a little bit more of the TdsParserStateObject file. My version splits the merge in smaller commits too.

So, I will make the PR and leave it up to you to decide what to do with it.

[bhugot]

@panoskj to bad didn't see your branch on this on your repo. I think you started the move and so your are more legit to it. If that can help I started moving TdsParser the same way. So after #1844 merge I could do the PR for TdsParser

[panoskj]

@panoskj to bad didn't see your branch on this on your repo.

@bhugot my bad, I should have added a link to it at least.

Anyway, I was trying to compare your version to mine, but it isn't so easy. It looks like we have a different position/order for some methods, which messes up the diff viewer. But I can see we have merged some different parts of the file (e.g. you have merged more snapshot related methods, but still I have merged more code in total).

We could merge both our versions, but since these PRs are already huge, it is probably best to go for a third PR along with the rest of the file. The important methods for fixing the async bug are already merged by both PRs anyway.

Thanks for the feedback on my PR by the way, I will check the code and come back with answers later.

PS: if you are working on TdsParser, I would say consider making smaller commits. I don't know what the reviewers prefer, but personally, I found it a bit hard to follow the first commit of this PR. Also try to keep the same order for the methods.