Skip to content

Conversation

@ccHanibal
Copy link
Contributor

As requested in #1305

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of comments and this LGTM.

}

[Test]
[TestCaseSource(nameof(CommitDateFormatTestCases))]
Copy link
Member

Choose a reason for hiding this comment

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

Since the test case formats are constant strings, please add them as regular [TestCase] attributes instead of the more complicated TestCaseData.

public string CommitDate
{
get { return _semver.BuildMetaData.CommitDate.UtcDateTime.ToString("yyyy-MM-dd"); }
get { return _semver.BuildMetaData.CommitDate.UtcDateTime.ToString(_config.CommitDateFormat); }
Copy link
Member

Choose a reason for hiding this comment

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

Is it perhaps a good idea to pass in CultureInfo.InvariantCulture here, just to avoid potential i18n problems or is it a "feature" that the date can be formatted in the locale of the process GitVersion is run under?

@ccHanibal
Copy link
Contributor Author

I adopted your advices. Please review them.

@asbjornu
Copy link
Member

Perfect! Can you please do a rebase of this branch on top of HEAD of master? This is how you do it:

git checkout master
git fetch upstream
git merge upstream/master
git checkout feature-commitdateformatconfigurable
git rebase master
git push --force

If you don't have the remote upstream defined, then do this before the above:

git remote add upstream git@github.com:GitTools/GitVersion.git

If you have any questions, please don't hesitate to ask. Thanks!

@ccHanibal
Copy link
Contributor Author

Sure I'll give it a try. Never done that before.

@ccHanibal ccHanibal force-pushed the feature-commitdateformatconfigurable branch from 0a73167 to dcc9c2b Compare October 14, 2017 16:14
@asbjornu
Copy link
Member

@ccHanibal: Looks like that worked. Thanks!

@asbjornu asbjornu merged commit 6d90d26 into GitTools:master Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants