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

date handling #4

Open
birbilis opened this issue Sep 19, 2018 · 6 comments
Open

date handling #4

birbilis opened this issue Sep 19, 2018 · 6 comments

Comments

@birbilis
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

birbilis commented Sep 20, 2018

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
Copy link
Owner

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

@birbilis
Copy link
Contributor Author

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
Copy link
Contributor Author

birbilis commented Oct 24, 2018

I mean the issue is at

return $"'{paramValue.ToString().Replace("'", "''")}'";

and it should do like in

sb.Append("'").Append(Convert.ToDateTime(dr[colIndex]).ToString("yyyy-MM-dd HH:mm")).Append("'");

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
Copy link
Contributor Author

...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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants