Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work towards merging TdsParser #2953

Open
MichelZ opened this issue Nov 2, 2024 · 3 comments
Open

Work towards merging TdsParser #2953

MichelZ opened this issue Nov 2, 2024 · 3 comments
Assignees
Labels
Common Project 🚮 Things that relate to the common project project

Comments

@MichelZ
Copy link
Contributor

MichelZ commented Nov 2, 2024

This is for tracking purposes connected to #1261
I would like to merge TdsParser to a single code base netfx/netcore

The approach I'd like to take is this:

  • Do small PR's first which only change code style to align netfx/netcore codebases
  • Do small PR's that target specific things (e.g. change from Types to type aliases e.g. Int32 to int)
  • Do more PR's that align code but do need code changes

This way (I hope?) changes are small-ish and/or low-riskish, so that they can be reviewed fairly fast (much easier than "big bang" PR's IMHO)

Hope that makes sense, if not just tell me please :)

#2839 also has an impact on this, because removal of SQL 7.0/2000 code means a lot less #IF NETFRAMEWORK constructs, so holding off with aliging SQL7.0/2000 specific code until we know if it needs to stay or can be removed

@edwardneal
Copy link
Contributor

Thanks for this @MichelZ - these bundles of changes look good to me.

Another pattern of changes will also need to be made at some point, across TdsParser and other core parts of SqlClient: Constrained Execution Region support exists in netfx, but was removed in netcore. This is (roughly) a no-op, but each instance adds ~45 lines to the diff and it's usually inside methods which are otherwise identical. It's probably not necessary as part of this cleanup, but it'd be a good idea to port the CER support to netcore before starting to merge identical methods.

On a wider point: I think it makes sense to use the TdsParser approach as a template for the core parts of SqlClient.

@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 2, 2024

I think that's it for now with the more trivial stuff. Once these are merged I'll continue with some more "meaty" one's :)
I do hope these are mostly straight forward to review and don't take up too much of anyone's time.

@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label Nov 4, 2024
@benrr101
Copy link
Contributor

benrr101 commented Nov 4, 2024

Thanks for taking a stab at merging this behemoth of a class 👍 I'll do my best to look at these PRs in a timely manner, but keep in mind we need two signoffs on them to get them merged. I'll also do my best to encourage my teammates to look at your PRs as well 😅

That being said, I think for the smaller changes, it might be more beneficial to combine a couple together - eg, comments and comment typos could be rolled into a single comment PR. I've been trying to structure my PRs such that each commit is a complete item. So, eg, if you had a PR for comments, one commit could be getting the blocks aligned, the second could be to fix the typos. You don't need to go back recall the PRs that are currently out, but it might be beneficial going forward. It's a delicate balance between too much overhead per PR and too much to review in a single PR.

My strategy for merging classes has been to create a "merge file" in the commit history of the PR. Basically indicate what blocks of code are from only one side of the merge. Then, merge work through each chunk of conflicts to either resolve as one or the other (eg, Int32 vs int) or leave with an #if NETFRAMEWORK sort of thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Common Project 🚮 Things that relate to the common project project
Projects
None yet
Development

No branches or pull requests

3 participants