Conversation
|
|
||
| @Override | ||
| public String toString() { | ||
| return dayOfWeek.getDisplayName(TextStyle.FULL, Locale.getDefault()) + " " + localTime.toString(); |
There was a problem hiding this comment.
Now that I'm looking at it, this probably shouldn't be locale-sensitive; if we want locale-sensitivity, that probably belongs in a dedicated "get for display" method that takes a Locale as an argument.
jodastephen
left a comment
jodastephen
left a comment
There was a problem hiding this comment.
Thanks for the PR. This is an initial review. I think all the basics are here, although it will need a full review later,
| private static final long SECONDS_PER_DAY = MILLIS_PER_DAY / 1000; | ||
| private static final long MINUTES_PER_DAY = SECONDS_PER_DAY / 60; | ||
|
|
||
| private DayTime(final DayOfWeek dayOfWeek, final LocalTime localTime) { |
There was a problem hiding this comment.
All methods should not use final on parameters
There was a problem hiding this comment.
In this project, even private methods have full Javadoc
| /** | ||
| * Obtains a {@code DayTime} with the given day of the week and local time. | ||
| * | ||
| * @param dayOfWeek the day of the week to represent; must not be {@code null} |
There was a problem hiding this comment.
Two spaces after parameter name: dayOfWeek the day -> dayOfWeek the day
; must not be {@code null} -> , not null
(same comment on all methods)
| * | ||
| * @param dayOfWeek the day of the week to represent; must not be {@code null} | ||
| * @param localTime the local time of day to represent; must not be {@code null} | ||
| * |
There was a problem hiding this comment.
No blank line between @param and @return
| return new DayTime(dayOfWeek, localTime); | ||
| } | ||
|
|
||
| public static DayTime from(final TemporalAccessor temporalAccessor) { |
There was a problem hiding this comment.
Oops. Wrong comment. Not done yet!
|
|
||
| import static org.junit.Assert.*; | ||
|
|
||
| public class DayTimeTest { |
|
Thanks for the review! That all sounds totally reasonable and doable, and I'll get on it shortly. I should also mention that my other broad goal is to raise test coverage; the current coverage is well below what I'm seeing in other classes in threeten-extra. |
|
This looks very useful. Is there anything I can do to help push this along? |
|
I have a lovely team of seven engineers. Would you mind managing them for a week or two? ;) In all seriousness, most of the work that remains is bringing unit tests in line with the standards of the rest of the threeten-extra project in terms of coverage and style. If you wanted to take a shot at that, it'd certainly be welcome! I do fully intend to finish this work as soon as time allows, for what it's worth. I'm afraid the opportunity has been slow to present itself, though. |
|
I was just about to pull-request an implementation of this concept, elaborating on Basil's answer to a question on Stack Overflow. But then I saw this, which looks very nice. |
|
Well, I think the assessment of the work that remains is still accurate. If you want to smooth out the tests, please feel free (mechanically, I think that would mean adding my fork as a remote, then branching off of my branch). Otherwise, I guess I'm really not going anywhere on the weekends these days, so given the reminder, I can probably just knock this out soon. |
Codecov Report
@@ Coverage Diff @@
## master #109 +/- ##
============================================
- Coverage 88.81% 87.99% -0.82%
- Complexity 2288 2320 +32
============================================
Files 57 58 +1
Lines 4290 4397 +107
Branches 609 622 +13
============================================
+ Hits 3810 3869 +59
- Misses 324 364 +40
- Partials 156 164 +8
Continue to review full report at Codecov.
|
|
I'm working on finishing the docs and tests this weekend and—as one would hope of writing docs and tests—the process is leading me to think more carefully about some decisions. I mentioned in an earlier comment that I had made some decisions for my own project about comparability that may not be universally-applicable. Looking at this again,
I'm not convinced there's a single right answer here, but I'd welcome perspective and suggestions. Thanks kindly! |
|
@jchambers Those are hard questions. It may seem intuitive to regard I think a linear approach of However, regarding the implementation of the |
4 participants
As discussed in #108, this introduces a
DayTimeclass that combines a day-of-week and time-of-day (e.g. "Monday at 13:45").I know I said I'd do some style-normalizing work first, but it occurred to me that there may some more structural things that need feedback, and so it's probably best to get an early round of review. I still fully intend to sort out the code style differences.
This closes #108.