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

Fix String.Format to handle formatting of multiple types correctly, not only DateTime #31

Merged
merged 3 commits into from
Apr 23, 2021

Conversation

DamienBraillard
Copy link
Contributor

Hi,

First, thanks for the hard work and the great library !

I started using Handlebars.Net and the associated Handlebars.Net.Helpers because I need to format values (which Stubble can't do easily). What I need to format is an "Instant" value representing a date from the "NodaTime" library.

I was quite surprised to get an error The method Format cannot be used on value '2001-11-21T00:00:00Z' of Type 'NodaTime.Instant'.

Looking into the code, it seems that only DateTime is supported:

[HandlebarsWriter(WriterType.String)]
public string Format(object value, string format)
{
    switch (value)
    {
        case DateTime dateTime:
            return dateTime.ToString(format, Context.Configuration.FormatProvider);

        default:
            throw new NotSupportedException($"The method {nameof(Format)} cannot be used on value '{value}' of Type '{value?.GetType()}'.");
    }
}

The method should make use of ICustomFormatter and IFormattable to format values as these interfaces are intended for this purpose (this is what the String.Format method uses to format string). Using these interfaces would allow formatting anything that can be like the String.Format method does.

Here is an example of a failing test that attempts to format a Decimal (But it is the same for NodaTime Instant):

[Fact]
public void TestFormat()
{
    // Arrange
    var data = new { MyValue = 1.2m };
    var template = @"{{String.Format MyValue ""N2""}}";

    var x = data.MyValue.ToString("N2");
    
    // Act
    var compiledTemplate = _target.PublicGetCompiledTemplate(template);

    // Assert
    var rendered = compiledTemplate(data);
    rendered.Should().Be("1.20");
}

Damien Braillard added 2 commits April 22, 2021 10:30
"String.Format" used to only support "DateTime" for formatting.

As for now, as soon as the value implements "IFormattable" that interface is used to perform the formatting. Also, if the format provider exposes a "ICustomFormatter" service, that formatter is used first for formatting the value.

The formatting falls back to "ToString()" or to an empty string if the value to format is null.
@DamienBraillard
Copy link
Contributor Author

If it's any help about the build failing, it seems that the task "Prepare analysis on SonarCloud" did not run because: "References service endpoint. PRs from repository forks are not allowed to access secrets in the pipeline. For more information see https://go.microsoft.com/fwlink/?linkid=862029"

This then caused the "SonarCloud: Run Code Analysis" task to fail because it was not prepared !

@StefH
Copy link
Collaborator

StefH commented Apr 22, 2021

@DamienBraillard . Thank you for the PR. I'll take a look.

About the CI-build failing, I don't know/see the difference between this CI build and this CI build (https://github.com/WireMock-Net/WireMock.Net/blob/master/azure-pipelines-ci.yml)

A PR on WireMock.Net does work correctly?
WireMock-Net/WireMock.Net#607

@0xced
Copy link
Contributor

0xced commented Apr 23, 2021

About the CI failure: The step Prepare analysis on SonarCloud did not run, eventually causing the SonarCloud: Run Code Analysis step to fail. The following warning gives a hint at what is happening:

References service endpoint. PRs from repository forks are not allowed to access secrets in the pipeline. For more information see https://go.microsoft.com/fwlink/?linkid=862029

How is you RUN_SONAR variable configured? Is it marked as secret in your Azure DevOps configuration?

See the Azure Pipelines documentation on Contributions from forks:

By default with GitHub pipelines, secrets associated with your build pipeline are not made available to pull request builds of forks. These secrets are enabled by default with GitHub Enterprise Server pipelines. Secrets include:

  • A security token with access to your GitHub repository.
  • These items, if your pipeline uses them:
    • Service connection credentials
    • Files from the secure files library
    • Build variables marked secret

@StefH
Copy link
Collaborator

StefH commented Apr 23, 2021

@0xced
No, only the MyGet key is secret, see:
image

@0xced
Copy link
Contributor

0xced commented Apr 23, 2021

The secret variables was not the cause. Searching for "PRs from repository forks are not allowed to access secrets in the pipeline" lead me to the source code of the Azure Pipeline agent runner. And the key in this error message was this: References service endpoint.

Apparently, the task is skipped because you have configured service connection credentials.

@StefH
Copy link
Collaborator

StefH commented Apr 23, 2021

@0xced
For my other project, I've this set:
image

And for this project, this checkbox is not enabled.

However, I think that enabling this checkbox is unsafe, like someone can see the secret MyGetKey ?

@0xced
Copy link
Contributor

0xced commented Apr 23, 2021

However, I think that enabling this checkbox is unsafe, like someone can see the secret MyGetKey ?

Indeed, your secret MyGetKey could be retrieved by someone forking your repository if you make secrets available to builds of forks and you don't want that.

Do you have a service connection configured? I think this could be the actual cause of the skipped task. The secret variables was just red herring.

overview-page

@StefH
Copy link
Collaborator

StefH commented Apr 23, 2021

Correct, I've a service connection:
image

@StefH
Copy link
Collaborator

StefH commented Apr 23, 2021

@DamienBraillard @0xced

I think for now I'll approve + merge this PR.

And update the pipeline yml to include ne(variables['Build.Reason'], 'PullRequest') for the sonarcloud and myget steps

@StefH StefH merged commit eda63bf into Handlebars-Net:master Apr 23, 2021
@StefH StefH added the enhancement New feature or request label Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants