DateRange does not include time #1904 #1905
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -304,33 +304,43 @@ public class DateRange | |
| /// Matches repositories with regards to the <param name="date"/>. | ||
| /// We will use the <param name="op"/> to see what operator will be applied to the date qualifier | ||
| /// </summary> | ||
| [Obsolete("This ctor has been deprecated as GitHub API now supports date and time")] | ||
|
There was a problem hiding this comment. Is it worth pointing out the preferred API and why you'd use it? Something like
There was a problem hiding this comment. Thanks @khellang good pickup. Just made both changes and checked-in. |
||
| public DateRange(DateTime date, SearchQualifierOperator op) : this(new DateTimeOffset(date), op) | ||
|
There was a problem hiding this comment. By forwarding these ctors to the new I think the safest thing to do is to keep the existing logic (as @ryangribble noted in #1904) for these ctors and use the new (and improved) format for the new |
||
| { | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Matches repositories with regards to both the <param name="from"/> and <param name="to"/> dates. | ||
| /// </summary> | ||
| [Obsolete("This ctor has been deprecated as GitHub API now supports date and time")] | ||
| public DateRange(DateTime from, DateTime to) : this(new DateTimeOffset(from), new DateTimeOffset(to)) | ||
| { | ||
| } | ||
|
|
||
| public DateRange(DateTimeOffset from, DateTimeOffset to) | ||
| { | ||
| query = string.Format(CultureInfo.InvariantCulture, "{0:yyyy-MM-dd'T'HH:mm:ss zzz}..{1:yyyy-MM-dd'T'HH:mm:ss zzz}", from.DateTime, to.DateTime); | ||
|
There was a problem hiding this comment. What will the There was a problem hiding this comment. Hi @khellang the However, you're right these changes affect the behaviour. I'll update and re-submit. There was a problem hiding this comment. But what is that offset? There was a problem hiding this comment. There was a problem hiding this comment. Hi sorry you are right @khellang. I was trying to do to code this while also doing my job which was a mistake. I'll fix in my next PR. |
||
| } | ||
|
|
||
| public DateRange(DateTimeOffset date, SearchQualifierOperator op) | ||
| { | ||
| switch (op) | ||
| { | ||
| case SearchQualifierOperator.GreaterThan: | ||
| query = string.Format(CultureInfo.InvariantCulture, ">{0:yyyy-MM-dd'T'HH:mm:ss zzz}", date); | ||
|
There was a problem hiding this comment. I'd probably store this pattern in a constant and reuse it throughout. There was a problem hiding this comment. Hi @khellang I've included this suggestion aswell. I've also updated the relevant unit tests to use a Theory which tests both the |
||
| break; | ||
| case SearchQualifierOperator.LessThan: | ||
| query = string.Format(CultureInfo.InvariantCulture, "<{0:yyyy-MM-dd'T'HH:mm:ss zzz}", date); | ||
| break; | ||
| case SearchQualifierOperator.LessThanOrEqualTo: | ||
| query = string.Format(CultureInfo.InvariantCulture, "<={0:yyyy-MM-dd'T'HH:mm:ss zzz}", date); | ||
| break; | ||
| case SearchQualifierOperator.GreaterThanOrEqualTo: | ||
| query = string.Format(CultureInfo.InvariantCulture, ">={0:yyyy-MM-dd'T'HH:mm:ss zzz}", date); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| internal string DebuggerDisplay | ||
| { | ||
| get { return string.Format(CultureInfo.InvariantCulture, "Query: {0}", query); } | ||
|
|
@@ -344,7 +354,7 @@ internal string DebuggerDisplay | |
| /// <returns><see cref="DateRange"/></returns> | ||
| public static DateRange LessThan(DateTime date) | ||
| { | ||
| return new DateRange(new DateTimeOffset(date), SearchQualifierOperator.LessThan); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -355,7 +365,7 @@ public static DateRange LessThan(DateTime date) | |
| /// <returns><see cref="DateRange"/></returns> | ||
| public static DateRange LessThanOrEquals(DateTime date) | ||
| { | ||
| return new DateRange(new DateTimeOffset(date), SearchQualifierOperator.LessThanOrEqualTo); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -366,7 +376,7 @@ public static DateRange LessThanOrEquals(DateTime date) | |
| /// <returns><see cref="DateRange"/></returns> | ||
| public static DateRange GreaterThan(DateTime date) | ||
| { | ||
| return new DateRange(new DateTimeOffset(date), SearchQualifierOperator.GreaterThan); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -377,7 +387,7 @@ public static DateRange GreaterThan(DateTime date) | |
| /// <returns><see cref="DateRange"/></returns> | ||
| public static DateRange GreaterThanOrEquals(DateTime date) | ||
| { | ||
| return new DateRange(new DateTimeOffset(date), SearchQualifierOperator.GreaterThanOrEqualTo); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -389,7 +399,7 @@ public static DateRange GreaterThanOrEquals(DateTime date) | |
| /// <returns><see cref="DateRange"/></returns> | ||
| public static DateRange Between(DateTime from, DateTime to) | ||
| { | ||
| return new DateRange(new DateTimeOffset(from), new DateTimeOffset(to)); | ||
| } | ||
|
|
||
| public override string ToString() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer the unit test actually tests the formatted string eg '2014-1-1'T'020406 1000' (or whatever it should be)
Otherwise these tests are susceptible to whatever formatting bug may exist inside
DateRangeclass.EG Im pretty sure the way you've implemented it at the moment isn't correctly formatting the string, yet all these tests pass (because you are using the
DateRangeclass to test theDateRangeclass! 🤣There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ryangribble
Has taken me a while to make these changes due to other commitments, however, they're now there.
Let me know what you think.