Skip to content

Formatting for VS Code change #3748

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Nov 15, 2024
Merged

Formatting for VS Code change #3748

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d252c34
Formatting for VS Code change
bleaphar Nov 6, 2024
5b2215c
Formatting for VS Code change
bleaphar Nov 6, 2024
33f5156
Merge branch 'dotnet:main' into main
bleaphar Nov 8, 2024
e209e8c
Merge branch 'main' of https://github.com/bleaphar/interactive
bleaphar Nov 14, 2024
b6b0d2c
Merge branch 'dotnet:main' into main
bleaphar Nov 14, 2024
3a64474
Merge branch 'main' of https://github.com/bleaphar/interactive
bleaphar Nov 14, 2024
a839ac4
Tweaks for dynamic expressions
bleaphar Nov 14, 2024
bda3eca
Change to UTC in test
bleaphar Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/DynamicExpressionUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;

using Microsoft.DotNet.Interactive.Parsing;

namespace Microsoft.DotNet.Interactive.Http.Parsing
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have unit tests that cover each of the formatting cases covered in this function?

{
// for $datetime, format the DateTime in order to eliminate the +00:00 offset and use Z
string text = currentDateTimeOffset.UtcDateTime.ToString(format, formatProvider);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a try catch around the ToString below, should there be one here too?


return node.CreateBindingSuccess(text);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may also make sense to extract a separate function for the try block above if you refactor the code to look like above...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's set up a little time for a group coding session.

}
}
else
{
Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.DotNet.Interactive.Http.Tests/ParserTests.DynamicExpressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public void can_bind_datetime_with_iso8601_format()
var code = $@"@var = {{{{{expression}}}}}""";

var result = HttpRequestParser.Parse(code);
var currentTime = DateTimeOffset.UtcNow;
var currentTime = DateTimeOffset.UtcNow.UtcDateTime;
var node = result.SyntaxTree.RootNode.DescendantNodesAndTokens().OfType<HttpExpressionNode>().Single();

var binding = DynamicExpressionUtilities.ResolveExpressionBinding(node, () => currentTime, expression);
Expand All @@ -66,7 +66,7 @@ public void can_bind_datetime_with_iso8601_format_with_offset()
var code = $@"@var = {{{{{expression}}}}}""";

var result = HttpRequestParser.Parse(code);
var currentTime = DateTimeOffset.UtcNow;
var currentTime = DateTimeOffset.UtcNow.UtcDateTime;
var node = result.SyntaxTree.RootNode.DescendantNodesAndTokens().OfType<HttpExpressionNode>().Single();

var binding = DynamicExpressionUtilities.ResolveExpressionBinding(node, () => currentTime, expression);
Expand Down
Loading