Formatting for VS Code change #3748
Conversation
| if (currentDateTimeOffset.Offset.TotalMinutes == 0) | ||
| { | ||
| // for $datetime, format the DateTime in order to eliminate the +00:00 offset and use Z | ||
| string text = currentDateTimeOffset.UtcDateTime.ToString(format, formatProvider); |
There was a problem hiding this comment.
There is a try catch around the ToString below, should there be one here too?
| // for $datetime, format the DateTime in order to eliminate the +00:00 offset and use Z | ||
| string text = currentDateTimeOffset.UtcDateTime.ToString(format, formatProvider); | ||
|
|
||
| return node.CreateBindingSuccess(text); |
There was a problem hiding this comment.
Not a huge deal but could we do something to consolidate the return statements for the success cases? There are many returns in this block and it makes the code a little difficult to read...
For example, consider changing it to something like below where each block computes a value for text and there is a single return statement at the end of the try.
try
{
string text;
if (...)
{
text = type.Value.ToString("r", CultureInfo.InvariantCulture;
}
else if (...)
{
text = type.Value.ToString("o", Thread.CurrentThread.CurrentUICulture);
}
...
else
{
var format = type.Value.Substring(1, type.Value.Length - 2);
text = type.Value.ToString(format, Thread.CurrentThread.CurrentUICulture);
}
return node.CreateBindingSuccess(text);
}
catch (FormatException)
{
return node.CreateBindingFailure(HttpDiagnostics.IncorrectDateTimeCustomFormat(format));
}
There was a problem hiding this comment.
It may also make sense to extract a separate function for the try block above if you refactor the code to look like above...
There was a problem hiding this comment.
Let's set up a little time for a group coding session.
| @@ -183,6 +184,13 @@ private static DateTimeOffset DefaultGetDateTimeOffset(bool isLocal) | |||
| else if (string.Equals(type.Value, "iso8601", StringComparison.OrdinalIgnoreCase)) | |||
| { | |||
| format = "o"; | |||
| if (currentDateTimeOffset.Offset.TotalMinutes == 0) | |||
There was a problem hiding this comment.
Do we have unit tests that cover each of the formatting cases covered in this function?
Introducing a change for datetime iso8601 so that it reflects the same format as visual studio code and reduces round tripping difficulties.