-
Notifications
You must be signed in to change notification settings - Fork 5
Description
Is there an existing issue for this?
- I have searched the existing open and closed issues
Description
Would it be worth changing SqlCommandSplitter to use https://github.com/microsoft/sqlmanagementobjects instead? It's used for SSMS and the like and is Microsoft-backed. The license is MIT, same as this repo, so there should be no issue with licensing.
It would remove the need to manage a SQL parser and batch splitter in this project.
Things to consider that I don't have a hard answer for:
- Does it matter that it's taking another dependency?
- Given
GOis not part of the SQL standard, shouldSqlCommandSplitteras it currently exists really be in dbup-core? Shouldn't the implementation that splits onGObe moved to dbup-sqlserver? - Does there even need to be a
SqlCommandSplitterat all? Can we get away with just putting the splitting code directly inDbUp.SqlServer.SqlConnectionManager.SplitScriptIntoCommands? It works in there and there are no other real uses ofSqlCommandSplitterthat I can see (aside from tests - which wouldn't be needed once the parsing and splitting logic was handed off to another package).
I can make a PR for it if it would be wanted. I've done most of the work already (there are a couple of tests that fail when using SMO, but those tests are inaccurate when compared to actual SSMS behaviour; I'm going to make a new issue about those) so it wouldn't be that much effort to tidy it up for PR.
What is the impact?
Chiefly, lowered maintenance burden for dbup-sqlserver and more reliable batch splitting.
It could be considered a breaking change because SMO will error on invalid SQL (which some of the inconsistent tests I mentioned earlier contain), but then it could also be argued that those scripts should never be allowed through in the first place, split into separate batches or not.
It would also correctly send multiple copies of a batch if a script ever specifies a repeat number after the batch separator.
If that is not considered a breaking change, there should be no immediately-noticeable effects to users, though the batch splitting will become more reliable (aside from an issue I have open for SMO at the moment, but that is currently an outstanding issue here as well)