Drizin / InterpolatedSql

Sql Builder using Interpolated Strings
MIT License
117 stars 7 forks source link

InterpolatedSql supports db string format hints on non-string parameters, fixes #8 #10

Closed terryaney closed 11 months ago

terryaney commented 11 months ago

Updated TransformArgument to support non-string types when adding dbType format hints.

Updated SqlTests

Drizin commented 11 months ago

Looks good, thanks a lot!!

I've made some minor changes (mostly to keep net462 compatibility) - please let me know if anything looks wrong.

Good catch on the IsAnsi bug, thanks!

terryaney commented 11 months ago

Only comment I have is, your original code (if I'm reading right), when argumentValue was null, it would end up hitting the code below (note, I didn't really understand what this else if was doing, I just 'ignored it' since I didn't feel I was changing behavior around it):

                else if (dbType == null && Enum.TryParse(value: testedFormat, ignoreCase: true, result: out parsedDbType))
                {
                    argumentValue = new DbTypeParameterInfo(null, argumentValue, direction, parsedDbType);
                }
                else
                {
                    matched = false;
                }

I tried to preserve that, but now if null it will process the argument if format is correct and turn argumentValue into a StringParameterInfo. I know you protect from null exception with your method using the null coallece...

        protected virtual string? TransformStringArgument(object? argValue)
        {
            return argValue is string v ? v : argValue?.ToString();
        }

So, behavior was changed. You know better than me if that is right or not.

terryaney commented 10 months ago

Hey @Drizin ,

Did you have any response to my concern above?

Additionally, there might be a problem with nuget. Your repo shows 2.1.2 for InterpolatedSql.Dapper:

image

But nuget seems to only know about 2.1.0?

image

Thanks, Terry

Drizin commented 10 months ago

The readme had a typo (wrong package). I have only republished the base library (InterpolatedSql), but not InterpolatedSql.Dapper. I wasn't sure if I should bump both when only one had a fix, so I didn't. So I guess you can just add a reference to InterpolatedSql and upgrade it individually.

Drizin commented 10 months ago

Regarding the change of behavior, I guess it was a bug - if the type is explicitly described (like varchar(200)) then I think it doesn't matter if the provided argument is null (does it make sense?). Probably I should improve the test coverage for that, but I guess it was just a bug. Thanks for following up, though.

terryaney commented 10 months ago

OK. When do you plan on updating nuget? Probably is pattern to easily switch from local build (if I pulled and built) to nuget ref that I don't quite have mastered ;)

terryaney commented 10 months ago

@Drizin Actually, before I just referenced InterpolatedSql.Dapper which pulls in InterpolatedSql automatically. Should I just add an additional reference to the (updated) InterpolatedSql package?

Drizin commented 10 months ago

Yep.