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

Add LocalTime.FromMinutesSinceMidnight #1366

Closed
DanCarterGMSL opened this issue May 1, 2019 · 3 comments
Closed

Add LocalTime.FromMinutesSinceMidnight #1366

DanCarterGMSL opened this issue May 1, 2019 · 3 comments
Assignees

Comments

@DanCarterGMSL
Copy link

@DanCarterGMSL DanCarterGMSL commented May 1, 2019

LocalTime has methods the following factory methods:

  • FromSecondsSinceMidnight()
  • FromMillisecondsSinceMidnight()
  • FromTicksSinceMidnight()

Could a FromMinutesSinceMidnight() method be added? It would be useful for something I am trying to do (we currently store a local time in minutes since midnight in our DB, so this method would make the conversion from DB types into NodaTime types easier).

A FromHoursSinceMidnight() method would complete the set, but I can't see it being as useful because you can just call the hour/minute ctor with 0 minutes.

@jskeet
Copy link
Member

@jskeet jskeet commented May 1, 2019

Yes, we can do this. Presumably you're now just using FromSecondsSinceMidnight(minutes * NodaConstants.SecondsPerHour) which seems reasonable to me. I'm not going to backport this to 2.x or anything like that, but we might as well make it simple.
I probably will include FromHoursSinceMidnight just for consistency. (In particular, that means it could be used in a method group conversion in a LINQ query - compare hoursList.Select(LocalTime.FromHoursSinceMidnight) vs hoursList.Select(h => new LocalTime(h, 0)) in terms of readability.)

Loading

@jskeet jskeet self-assigned this May 1, 2019
@DanCarterGMSL
Copy link
Author

@DanCarterGMSL DanCarterGMSL commented May 1, 2019

NodaConstants.SecondsPerMinute, but yes!

Loading

@jskeet
Copy link
Member

@jskeet jskeet commented May 1, 2019

Doh :) (And that's why it's worth having as a method, of course, to prevent stupid mistakes like that...)

Loading

jskeet added a commit to jskeet/nodatime that referenced this issue May 1, 2019
Fixes nodatime#1366

(FromHoursSinceMidnight is a little less useful, but I've included it for consistency/completeness.)
jskeet added a commit to jskeet/nodatime that referenced this issue May 4, 2019
Fixes nodatime#1366

(FromHoursSinceMidnight is a little less useful, but I've included it for consistency/completeness.)
@jskeet jskeet closed this in #1367 May 4, 2019
jskeet added a commit that referenced this issue May 4, 2019
Fixes #1366

(FromHoursSinceMidnight is a little less useful, but I've included it for consistency/completeness.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants