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 LocalDate.{Min,Max}Value #898

Closed
roji opened this Issue Jul 7, 2017 · 15 comments

Comments

Projects
None yet
3 participants
@roji

roji commented Jul 7, 2017

Much like #356 but for LocalDate.

@jskeet

This comment has been minimized.

Member

jskeet commented Jul 7, 2017

SGTM - do you think it's reasonable to throw an exception if they use different calendar systems, just like CompareTo does?

I think it would be reasonable to get this in for 2.1 (currently in beta). Do you have anything else you'd like for Postgres support? (It could be a while before 2.2...)

@roji

This comment has been minimized.

roji commented Jul 7, 2017

(I'm not yet very familiar with how calendar systems and LocalDate interact) If I understand you right, you're saying that {Min,Max}Values would be tied to a specific calendar system (because we can't talk about a system-independent maximum value) - that definitely makes sense. But which operation exactly would throw an exception and when? If I understand correctly CompareTo already does so?

Regardless, there's an Npgsql feature which relies on this, but I can't say it's extremely important. In case you're interested, PostgreSQL date and timestamp support special infinity and negative infinity values in the database, and Npgsql provides an option to convert {Max,Min}Value to these. I'm doing this for timestamp but ran into the lack of {Max,Min}Value for dates. This is definitely non-critical.

Otherwise there's nothing more at the moment that seems to be lacking for me - it's quite exciting to be able to provide NodaTime types for Npgsql via a plugin. I'll write a blog post about it soon.

@jskeet

This comment has been minimized.

Member

jskeet commented Jul 7, 2017

Ah, sorry, I'd not paid enough attention - I thought you meant methods of

LocalDate Max(LocalDate, LocalDate)
LocalDate Min(LocalDate, LocalDate)

Those sound useful as well :)

For MaxValue and MinValue properties... I think we could easily expose MaxIsoValue and MinIsoValue, which are tied to the ISO (Gregorian) calendar. I suspect that's all you need.

(And I agree that having support in Npgsql will be exciting. I look forward to the blog post!)

@roji

This comment has been minimized.

roji commented Jul 7, 2017

MaxIsoValue and MinIsoValue would be perfect. For completeness it might be a good idea to have a way of obtaining max/min values for other calendar systems as well, but that's definitely not something I need myself.

Max/Min methods for pairs of dates also sound useful.

@jskeet jskeet added the enhancement label Jul 7, 2017

@jskeet jskeet added this to the 2.1-consider milestone Jul 7, 2017

@jskeet jskeet self-assigned this Jul 7, 2017

@jskeet

This comment has been minimized.

Member

jskeet commented Jul 7, 2017

Righto. Will try to do that this weekend.

@malcolmr

This comment has been minimized.

Contributor

malcolmr commented Jul 7, 2017

I went looking to see if we ever had these (e.g. pre-1.0), but it looks like we didn't. I guess we didn't want to tie ourselves to the calendar system. (And I think our story about valid ranges was a bit muddled pre-2.0.)

Have something that's clearly ISO-only makes sense to me too.

(e.g. we already needed a MinIsoDate / MaxIsoDate in DateIntervalTest.cs; I'm assuming those would be the same values?)

@jskeet

This comment has been minimized.

Member

jskeet commented Jul 7, 2017

Yup. For other calendar systems, I think it makes sense to have MinDate and MaxDate properties on CalendarSystem. We have MaxYear and MinYear, so it's not much of a stretch from there. I think those can go post-2.1 though.

@malcolmr

This comment has been minimized.

Contributor

malcolmr commented Jul 7, 2017

I do have a chat log with you mentioning in passing possible problems with static initialisation order in reference to a potential LocalDate.MinValue, though, but I don't have any other context.

Any idea whether that's a real concern?

(It was about whether we had a test that verified the class initialisation formed a DAG.)

@jskeet

This comment has been minimized.

Member

jskeet commented Jul 7, 2017

If we make it a property instead of a field, it should be fine.

jskeet added a commit to jskeet/nodatime that referenced this issue Jul 7, 2017

jskeet added a commit to jskeet/nodatime that referenced this issue Jul 7, 2017

jskeet added a commit to jskeet/nodatime that referenced this issue Jul 7, 2017

@jskeet jskeet closed this in #899 Jul 8, 2017

@roji

This comment has been minimized.

roji commented Jul 8, 2017

Great, thanks!

Any estimate on when 2.1.0 is expected to be released? Just so I coordinate my own Nodatime plugin release etc.

@jskeet

This comment has been minimized.

Member

jskeet commented Jul 8, 2017

This weekend, hopefully - but certainly within the week. To put it another way, if it isn't released very soon, please bug me :)

@roji

This comment has been minimized.

roji commented Jul 8, 2017

OK great, I wasn't planning on releasing on my side for a bit, so no issue.

@roji

This comment has been minimized.

roji commented Jul 9, 2017

FYI am using the recently-released 2.1.0 and LocalDate.{Max,Min}IsoDate are working great, thanks!

@jskeet

This comment has been minimized.

Member

jskeet commented Jul 9, 2017

Excellent - glad to hear it :) (Always nice to know that a new release works at all, to be honest...)

@roji

This comment has been minimized.

roji commented Jul 9, 2017

Well I wouldn't count on my budding meager test suite to confirm anything, but yeah, tests pass. Am impressed with my luck submitting this request just before the release...

@malcolmr malcolmr modified the milestones: 2.1.0, 2.1-consider Aug 6, 2017

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