DateRange does not include time #1904 #1905

patricknolan · 2018-11-05T04:58:37Z

Hi @ryangribble please let me know what you think.

patricknolan · 2018-11-05T05:33:19Z

Hi @ryangribble build failed but cause does'nt seem to be related to my changes.

shiftkey · 2018-11-05T12:17:12Z

@patricknolan I restarted the problem build - let's see if it goes green

khellang · 2018-11-05T13:33:28Z

Octokit/Models/Request/SearchRepositoriesRequest.cs

@@ -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>
- public DateRange(DateTime date, SearchQualifierOperator op)
+ [Obsolete("This ctor has been deprecated as GitHub API now supports date and time")]
+ public DateRange(DateTime date, SearchQualifierOperator op) : this(new DateTimeOffset(date), op)


By forwarding these ctors to the new DateTimeOffset overloads, the behavior has changed by taking the time into account. That could be a breaking change.

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 DateTimeOffset ctors.

khellang · 2018-11-05T13:37:55Z

Octokit/Models/Request/SearchRepositoriesRequest.cs

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


What will the zzz component be here, given you're using the DateTime property and not the DateTimeOffset itself?

Hi @khellang the zzz includes the timezone offset which I believe is correct.

However, you're right these changes affect the behaviour. I'll update and re-submit.

But what is that offset? DateTime doesn't store a separate offset.

See https://docs.microsoft.com/en-us/dotnet/api/system.datetimeoffset.datetime?view=netframework-4.7.2#remarks

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.

khellang · 2018-11-05T15:06:32Z

Octokit/Models/Request/SearchRepositoriesRequest.cs

 {
 switch (op)
 {
 case SearchQualifierOperator.GreaterThan:
- query = string.Format(CultureInfo.InvariantCulture, ">{0:yyyy-MM-dd}", date);
+ query = string.Format(CultureInfo.InvariantCulture, ">{0:yyyy-MM-dd'T'HH:mm:ss zzz}", date);


I'd probably store this pattern in a constant and reuse it throughout.

Hi @khellang I've included this suggestion aswell.

I've also updated the relevant unit tests to use a Theory which tests both the DateTime and DateTimeOffset strategies.

patricknolan · 2018-11-05T20:50:02Z

Hi @ryangribble and @khellang when you get a chance let me know what you think!

khellang

Just a few nits from me 👍

khellang · 2018-11-05T21:14:23Z

Octokit/Models/Request/SearchRepositoriesRequest.cs

@@ -298,37 +298,66 @@ public override string ToString()
 [DebuggerDisplay("{DebuggerDisplay,nq}")]
 public class DateRange
 {
+ public const string DateTimePatten = "yyyy-MM-dd'T'HH:mm:ss zzz";
+ public const string DatePatten = "yyyy-MM-dd";


Nit: Patten -> Pattern

khellang · 2018-11-05T21:16:10Z

Octokit/Models/Request/SearchRepositoriesRequest.cs

 private readonly string query = string.Empty;

 /// <summary>
 /// 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")]


Is it worth pointing out the preferred API and why you'd use it? Something like

This constructor doesn't use the time component of the specified DateTime. Please use the overload accepting a DateTimeOffset, which also supports time.

Thanks @khellang good pickup. Just made both changes and checked-in.

patricknolan · 2018-11-05T22:49:34Z

Hi @khellang all done.

khellang · 2018-11-05T23:00:05Z

Octokit/Models/Request/SearchRepositoriesRequest.cs

 public DateRange(DateTime date, SearchQualifierOperator op)
 {
 switch (op)
 {
 case SearchQualifierOperator.GreaterThan:
- query = string.Format(CultureInfo.InvariantCulture, ">{0:yyyy-MM-dd}", date);
+ query = string.Format(CultureInfo.InvariantCulture, ">{0:DatePatten}", date);


Does this compile? 🤔

I'm thinking it might be better to just use ToString(DatePattern, CultureInfo.InvariantCulture) and string concatenation or interpolation than the current format strings...

Hi @khellang you're thinking something like this?

query = $"{from.ToString(DateTimePattern, CultureInfo.InvariantCulture)}..{to.ToString(DateTimePattern, CultureInfo.InvariantCulture)}";

I'm thinking something like query = FormattableString.Invariant($"{from.ToString(DateTimePattern)}..{to.ToString(DateTimePattern)}"); 😄

khellang · 2018-11-05T23:02:28Z

Octokit/Models/Request/SearchRepositoriesRequest.cs

@@ -342,6 +371,7 @@ internal string DebuggerDisplay
 /// </summary>
 /// <param name="date">date to be used for comparison (times are ignored)</param>
 /// <returns><see cref="DateRange"/></returns>
+ [Obsolete("This ctor has been deprecated as GitHub API now supports date and time")]


These are not ctors. Maybe also change these to reflect the recommended APIs?

ryangribble

Hi @patricknolan
Thanks for sending the PR over, and thanks @khellang for starting the review.

Just to confirm, yes we dont want any behaviuor change on the exiusting DateTime based ctors and methods. As you've done we can add DateTimeOffset versions of everything and flag the old ones as [Obsolete]. They will be deleted from the code base in 2 releases time.

In terms of the tests, im not sure I like the complexity of adding the enum and switching out the request code, as once you take my other comment into account (and actually test the formatted date/time string) there will be a 2nd point that needs to switch. IMO it would be better to just leave all the old tests completely untouched and just copy them for DateTimeOffset versions. As I say, the deprecated stuff will be removed in a couple of releases time so I dont think the test complexity needs to be introduced, would rather just copy/paste them for now, and remove the old ones later.

In terms of the [Obsolete] messages, can we use these please?

"This ctor does not support the time component or timezone and will be removed in a future release. Please use the DateTimeOffset overload instead"

"This method does not support the time component or timezone and will be removed in a future release. Please use the DateTimeOffset overload instead"

ryangribble · 2018-11-06T12:35:35Z

Octokit/Models/Request/SearchRepositoriesRequest.cs

+ [Obsolete("This constructor doesn't use the time component of the specified DateTime. Please use the overload accepting a DateTimeOffset, which also supports time.")]
+ public DateRange(DateTime from, DateTime to) 
+ {
+ query = string.Format(CultureInfo.InvariantCulture, "{0:DateTimePattern}..{1:DateTimePattern}", from, to);


Same here, it needs to use Date only, not Date+Time

ryangribble · 2018-11-06T12:36:43Z

Octokit/Models/Request/SearchRepositoriesRequest.cs

 {
- query = string.Format(CultureInfo.InvariantCulture, "{0:yyyy-MM-dd}..{1:yyyy-MM-dd}", from, to);
+ query = string.Format(CultureInfo.InvariantCulture, "{0:DateTimePatten}..{1:DateTimePatten}", from, to);


This still has the old spelling. Im not sure you even can parameterize the format string like this anyway?!

ryangribble · 2018-11-06T12:42:28Z

Octokit.Tests/Clients/SearchClientTests.cs

 connection.Received().Get<SearchUsersResult>(
 Arg.Is<Uri>(u => u.ToString() == "search/users"),
- Arg.Is<Dictionary<string, string>>(d => d["q"] == "github+created:>2014-01-01"));
+ Arg.Is<Dictionary<string, string>>(d => d["q"] == $"github+created:{request.Created}"));


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 DateRange class.

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 DateRange class to test the DateRange class! 🤣

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.

patricknolan · 2019-03-14T08:55:49Z

Hi @ryangribble and @khellang,,

Has taken me a while to make changes due to other commitments, however, I have now made the changes advised.

Let me know what you think.

ryangribble · 2019-03-14T10:01:39Z

👋 Hi @patricknolan

Thanks for picking this back up! I'm just trying to get back up to speed... The changes look pretty good now.

Just on the [Obsolete] messages, I can see previously I suggested that this wording might be more clear... what do you think?

In terms of the [Obsolete] messages, can we use these please?

"This ctor does not support the time component or timezone and will be removed in a future release. Please use the DateTimeOffset overload instead"

"This method does not support the time component or timezone and will be removed in a future release. Please use the DateTimeOffset overload instead"

octokit#1904 octokit#1905

patricknolan · 2019-03-14T20:50:35Z

Hi @ryangribble changes for [Obsolete] messages have been done. The CI failed, however, it may be an intermittent error as I don't believe the changes could have caused this to fail. I'm not sure how to invoke the CI again without an additional check-in?

ryangribble · 2019-03-14T22:26:17Z

As a user you can't rerun them, I've kicked them off again

patricknolan · 2019-03-18T02:41:37Z

Thanks @ryangribble. You ok for this to be merged now?

ryangribble · 2019-03-19T11:03:05Z

Yep I think we're good now! Thanks @patricknolan 👍 🚢

patricknolan · 2019-03-28T09:06:28Z

Thanks @ryangribble do you know when this update will be available via nuget?

ryangribble · 2019-03-30T03:03:37Z

release_notes: Enhanced DateRange class used in SearchClient requests to support time and timezone components

patricknolan · 2019-04-24T04:55:22Z

Hi @ryangribble just confirming if you do the release notes or do I? Sorry this is my first contribution no am not sure.

Am keen to get a new release available via Nuget.

ryangribble · 2019-04-24T09:25:44Z

Hi @patricknolan sorry for the confusion, this is something I do... I'm really sorry I've just been tied up in lots of real life stuff and not cut a new release as yet. If you add a custom nuget source of our appveyor feed https://ci.appveyor.com/nuget/octokit-net, you can get the latest "beta" packages from our master branch, but yes I really do need to make a release and push to nuget proper 🙏

patricknolan · 2019-04-25T09:15:41Z

Not a problem @ryangribble completely understand

DateRange does not include time octokit#1904

2b06cc9

khellang approved these changes Nov 5, 2018

View reviewed changes

khellang reviewed Nov 5, 2018

View reviewed changes

DateRange does not include time octokit#1904

86a2d5f

khellang approved these changes Nov 5, 2018

View reviewed changes

DateRange does not include time octokit#1904

2bc9ba6

khellang reviewed Nov 5, 2018

View reviewed changes

patricknolan added 2 commits November 6, 2018 09:37

DateRange does not include time octokit#1904

65435b9

DateRange does not include time octokit#1904

17e94a8

ryangribble suggested changes Nov 6, 2018

View reviewed changes

Requested changed by ryangribble

a02f466

Update to Obselete Attribute messages

0051e9c

octokit#1904 octokit#1905

ryangribble merged commit d16ac98 into octokit:master Mar 19, 2019

ryangribble added the category: feature label Mar 29, 2019

shiftkey mentioned this pull request Sep 10, 2019

DateRange does not include time #1904

Closed

nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022

DateRange does not include time #1904 #1905

DateRange does not include time #1904 #1905

Conversation

patricknolan commented Nov 5, 2018

patricknolan commented Nov 5, 2018

shiftkey commented Nov 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khellang Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricknolan commented Nov 5, 2018

khellang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricknolan commented Nov 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khellang Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryangribble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricknolan commented Mar 14, 2019

ryangribble commented Mar 14, 2019 • edited Loading

patricknolan commented Mar 14, 2019

ryangribble commented Mar 14, 2019 • edited Loading

patricknolan commented Mar 18, 2019

ryangribble commented Mar 19, 2019

patricknolan commented Mar 28, 2019

ryangribble commented Mar 30, 2019

patricknolan commented Apr 24, 2019

ryangribble commented Apr 24, 2019

patricknolan commented Apr 25, 2019

khellang Nov 5, 2018 •

edited

Loading

khellang Nov 7, 2018 •

edited

Loading

ryangribble commented Mar 14, 2019 •

edited

Loading

ryangribble commented Mar 14, 2019 •

edited

Loading