fluentmigrator icon indicating copy to clipboard operation
fluentmigrator copied to clipboard

Bugfix/1827 raw sql

Open PhenX opened this issue 1 year ago • 8 comments

A proposal to fix the #1827 issue

PhenX avatar Oct 04 '24 20:10 PhenX

@PhenX The Postgres tests are failing due to the new code missing a trailing semi-colon at the end of the statement. Any chance you can fix it? I think we should include the semi-colon, to be clear, which I believe was the previous behavior. One reason is preview-only mode scripts need a way to disambiguate grammar when used with ambiguous grammar keywords that start an Execute.Sql block.

jzabroski avatar Oct 10 '24 19:10 jzabroski

@PhenX The Postgres tests are failing due to the new code missing a trailing semi-colon at the end of the statement. Any chance you can fix it? I think we should include the semi-colon, to be clear, which I believe was the previous behavior. One reason is preview-only mode scripts need a way to disambiguate grammar when used with ambiguous grammar keywords that start an Execute.Sql block.

Yes I feared this detail would fail tests. I'll change the impacted expression. Did I do the right thing removing the overloads that did the same as the base class?

PhenX avatar Oct 10 '24 19:10 PhenX

Did I do the right thing removing the overloads that did the same as the base class?

I guess it depends if that is why it is breaking. I am working on a PR to add an ISeparator interface to FluentMigrator to separate statements and statement batches. If we had that, we may not need customization per generator?

jzabroski avatar Oct 10 '24 20:10 jzabroski

@jzabroski I pushed the changes, but had to change a few tests for spacing and the condition "1 = 1" present for all other dbms, is that a problem? Also, when does the pipeline run the checks?

PhenX avatar Oct 13 '24 20:10 PhenX

There's some problem with the pipeline that appears to have started when I renamed the master branch to main. It seems like AzureDevOps still defaults to assuming master in many places.

i have been manually running builds against each pr head commit. Would love to figure this out.

jzabroski avatar Oct 13 '24 20:10 jzabroski

My take on the Where 1 = 1 is it's an idiom used to silence static analysis tools like RedGate SQL Prompt that warn when a DML statement is missing a where clause. In this sense it makes sense to generate it for .AllRows()

jzabroski avatar Oct 13 '24 22:10 jzabroski

I kicked off a CI build for this last commit.

jzabroski avatar Oct 15 '24 20:10 jzabroski

I kicked off a CI build for this last commit.

Oh 😅 looks like I'm not done yet

PhenX avatar Oct 16 '24 05:10 PhenX