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 a DayTime Temporal #108

Open
jchambers opened this issue Sep 22, 2018 · 6 comments · May be fixed by #109
Open

Add a DayTime Temporal #108

jchambers opened this issue Sep 22, 2018 · 6 comments · May be fixed by #109
Labels

Comments

@jchambers
Copy link

I've recently built a DayTime Temporal that represents a combination of a day of the week and a time of day (e.g. "Monday at 13:45"). I just discovered this project (thanks for this and all of your other work on all things Java time-related!); are you interested in a DayTime contribution?

Because I wasn't aware of this project when I built my implementation, it'll take some work to adjust the code style (and testing style?) to match this project's conventions, and so I wanted to make sure it's something you actually want before I dig in.

Cheers!

@jodastephen
Copy link
Member

That seems entirely within the scope of this project :-) Is it day-of-week or day-of-month? I assume the former. Normally 310 uses "Day" for day-of-month" as in MonthDay, but DayOfWeekTime is a bit of a mouthful...

@jchambers
Copy link
Author

It is, indeed, day-of-week.

I went through the same thought process and (being less bound by convention because it wasn't part of 310) figured that day-of-month/time-of-day probably wasn't a combination a lot of people would ever really use, and so if anything deserved an awkward name, it would be the awkward concept (e.g. DayOfMonthTime).

Happy to rename as needed, though!

@jodastephen
Copy link
Member

Lets go with DayTime. Day-of-month + time just seems so rare I don't think we'd want to even add it here.

@jchambers jchambers linked a pull request Sep 24, 2018 that will close this issue
@jchambers
Copy link
Author

@jodastephen I've opened #109. I'm jumping the gun a little bit and opened the pull request before resolving style differences to get some early feedback and catch any structural concerns you may have as soon as possible. I do fully intend to resolve those differences, though.

Cheers!

@joshuaali
Copy link

If this does get implemented, perhaps a DayTimeRange (Monday 18:00 - Tuesday 02:00) could also be introduced - analogous to LocalDateRange and Interval.

The use case I have in mind is for business hours, which are often the same week-to-week.

@jchambers
Copy link
Author

I do, in fact, have an implementation of something like DayTimeRange in my own project and for exactly the use case (business hours) that you describe.

I'm a little hesitant to try to make it The One True Way To Do Things, though, since there are a lot of gnarly design questions to answer when dealing with cyclic intervals. "Beforeness" and "afterness" are particularly dicey, for example.

That isn't to say it can't be done, of course! I made choices that made sense for my particular project, though, and going through the process made me realize how many different "right" answers there might be for different people. I'd want to have a pretty clear shared understanding of goals before starting in on a project to cook up an implementation intended for general use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants