Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Adds new Date Format String for HttpRuleParser (#507) #897

Merged
merged 1 commit into from
Jul 18, 2017
Merged

Adds new Date Format String for HttpRuleParser (#507) #897

merged 1 commit into from
Jul 18, 2017

Conversation

jkotalik
Copy link
Contributor

@jkotalik jkotalik requested a review from Tratcher July 13, 2017 18:21
@jkotalik jkotalik changed the title Fix to #507. Adds new Date Format String for HttpRuleParser (#507) Jul 13, 2017
@Tratcher Tratcher added this to the 2.1.0 milestone Jul 14, 2017
@Tratcher
Copy link
Member

CLA required? @Eilon was it just the public one? https://github.com/aspnet/Home/blob/master/CONTRIBUTING.md#contributing-code-and-content

@Eilon
Copy link
Contributor

Eilon commented Jul 16, 2017

I worked it out with @jkotalik so I think he'll be fine once he completes the process.

Eilon
Eilon suggested changes Jul 16, 2017
View reviewed changes
test/Microsoft.Net.Http.Headers.Tests/DateParserTest.cs Outdated
CheckValidParsedValue("Tue, 15 Nov 1994 08:12:31 GMT", new DateTimeOffset(1994, 11, 15, 8, 12, 31, TimeSpan.Zero));
CheckValidParsedValue(" Sunday, 06-Nov-94 08:49:37 GMT ", new DateTimeOffset(1994, 11, 6, 8, 49, 37, TimeSpan.Zero));
CheckValidParsedValue(" Tue,\r\n 15 Nov\r\n 1994 08:12:31 GMT ", new DateTimeOffset(1994, 11, 15, 8, 12, 31, TimeSpan.Zero));
CheckValidParsedValue("Sat, 09-Dec-2017 07:07:03 GMT ", new DateTimeOffset(2017, 12, 09, 7, 7, 3, TimeSpan.Zero));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should change this to a Theory test - one for each case.

Tratcher
Tratcher approved these changes Jul 17, 2017
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

Pending comments and CLA

test/Microsoft.Net.Http.Headers.Tests/DateParserTest.cs Outdated
CheckValidParsedValue("Tue, 15 Nov 1994 08:12:31 GMT", new DateTimeOffset(1994, 11, 15, 8, 12, 31, TimeSpan.Zero));
CheckValidParsedValue(" Sunday, 06-Nov-94 08:49:37 GMT ", new DateTimeOffset(1994, 11, 6, 8, 49, 37, TimeSpan.Zero));
CheckValidParsedValue(" Tue,\r\n 15 Nov\r\n 1994 08:12:31 GMT ", new DateTimeOffset(1994, 11, 15, 8, 12, 31, TimeSpan.Zero));
CheckValidParsedValue(input, expected);
Copy link
Member

Choose a reason for hiding this comment

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

You don't still need CheckValidParsedValue and CheckInvalidParsedValue do you?

@dnfclas
Copy link

dnfclas commented Jul 17, 2017

@jkotalik, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

@jkotalik
Copy link
Contributor Author

@Eilon 🆙 📅

Eilon
Eilon approved these changes Jul 17, 2017
View reviewed changes
Copy link
Contributor

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

Approved for style, but I didn't look at the actual code.

test/Microsoft.Net.Http.Headers.Tests/DateParserTest.cs Outdated
{
[Fact]
public void TryParse_SetOfValidValueStrings_ParsedCorrectly()
[Theory, MemberData(nameof(ValidStringData))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Per my comment in some other PR, split the attributes into two lines, each like [Foo].

test/Microsoft.Net.Http.Headers.Tests/DateParserTest.cs Outdated
yield return new object[] { "Sat, 09-Dec-2017 07:07:03 GMT ", new DateTimeOffset(2017, 12, 09, 7, 7, 3, TimeSpan.Zero) };
}

[Theory, MemberData(nameof(InvalidStringData))]
Copy link
Contributor

Choose a reason for hiding this comment

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

And this one.

@jkotalik jkotalik force-pushed the jkotalik/DateParsing branch from 839ee22 to 538cbf0 Compare July 18, 2017 16:15
@jkotalik
Copy link
Contributor Author

@Tratcher Went ahead and refactored the helpers. Will wait for this build to succeed to merge in.

@jkotalik jkotalik force-pushed the jkotalik/DateParsing branch from 5370edd to 8bc2e53 Compare July 18, 2017 18:33
@jkotalik jkotalik force-pushed the jkotalik/DateParsing branch from 8bc2e53 to b41d865 Compare July 18, 2017 20:35
@jkotalik jkotalik merged commit b41d865 into dev Jul 18, 2017
@jkotalik jkotalik deleted the jkotalik/DateParsing branch July 31, 2017 01:11
@jkotalik jkotalik mentioned this pull request Jul 31, 2017
Closed
@Tratcher Tratcher mentioned this pull request May 15, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@Eilon Eilon Eilon approved these changes

@Tratcher Tratcher Tratcher approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jkotalik jkotalik

Labels

None yet

Projects

None yet

Milestone

2.1.0

Development

Successfully merging this pull request may close these issues.

4 participants

@jkotalik @Tratcher @Eilon @dnfclas