jphellemons / CommandAsSql

It's a simple extension method which enables you to write out a parsed SqlCommand object with their parameters and containing values
MIT License
18 stars 7 forks source link

date handling #4

Open birbilis opened 6 years ago

birbilis commented 6 years ago

Do you feel the retval = "'" + sp.Value.ToString().Replace("'", "''") + "'"; is correct for DateTime?

I'm getting localized Dates in Greek when I use it via CommandAsSql library.

In the library code it also uses later on sb.Append("'").Append(Convert.ToDateTime(dr[colIndex]).ToString("yyyy-MM-dd HH:mm")).Append("'"); for handling structure types which seems more logical to use everywhere

I mean the .ToString("yyyy-MM-dd HH:mm") - not sure about the DateTimeOffset and DateTime2, guess for those too

birbilis commented 6 years ago

if you see https://github.com/jeroenpot/SqlHelper/blob/master/Source/Mirabeau.MsSql.Library/SqlGenerator.cs

it has in GetParameterValue the following:

            case SqlDbType.Date:
            case SqlDbType.DateTime:
            case SqlDbType.DateTime2:
            case SqlDbType.DateTimeOffset:
                  var dateTime = ((DateTime)sqlParameter.Value)
                                                    .ToString("yyyy-MM-dd HH:mm:ss:fff", FormatCulture);
                 retval = string.Format(FormatCulture, "convert(datetime,'{0}', 121)", dateTime);
                  break;

so probably it is wrong as is currently in this library

also should check how they handle some of their other parameters differently in that method and compare with the implementation in this library to merge / keep the most types and use the correct implementation (not assuming theirs is always the correct one)

birbilis commented 6 years ago

btw, they also use :ss:fff but that other code in this library that I had mentioned (which is similar to their version) doesn't include the :ss:fff - so not sure if one should add those two parts in the output date (or maybe the ss:fff means to not output if they're zero, don't remember)

also, not sure what this code of theirs does: string.Format(FormatCulture, "convert(datetime,'{0}', 121)", dateTime);

jphellemons commented 6 years ago

121 = yyyy-mm-dd hh:mi:ss.mmm

birbilis commented 6 years ago

yep, that's the format I'd expect to see, but the code in the library currently just spits out the date in localized format cause of .ToString() instead of using .ToString("yyyy-MM-dd HH:mm"))

birbilis commented 6 years ago

I mean the issue is at https://github.com/jphellemons/CommandAsSql/blob/97f3bd7d4aa6850c2ff8f28fa9c7804e82e38786/CommandAsSql/ExtensionMethods.cs#L98

and it should do like in https://github.com/jphellemons/CommandAsSql/blob/97f3bd7d4aa6850c2ff8f28fa9c7804e82e38786/CommandAsSql/ExtensionMethods.cs#L126

that is: return $"'{paramValue.ToString("yyyy-MM-dd HH:mm").Replace("'", "''")}'"; but not sure if that .Replace("'", "''") is needed, so maybe that should be removed

plus do that at a separate case statement for the following ones (now they were together with other cases in the switch at ParameterValueForSQL method): case SqlDbType.Date: case SqlDbType.DateTime: case SqlDbType.DateTime2: case SqlDbType.DateTimeOffset:

not sure for the Date one (maybe should leave that as is). Also I'm not familiar with DateTime2 and especially DateTimeOffset types, but for the DateTime one, should definitely use a separate case and format the date instead of spitting out a localized one

birbilis commented 6 years ago

...the https://github.com/jeroenpot/SqlHelper/blob/master/Source/Mirabeau.MsSql.Library/SqlGenerator.cs I mentioned above does it for all those types, however what it does looks too complex to me (not sure of the rationale behind it)