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

Simplify LocalDate(Time) constructor parameter names #84

Open
fluidsonic opened this issue Dec 15, 2020 · 7 comments · May be fixed by #387
Open

Simplify LocalDate(Time) constructor parameter names #84

fluidsonic opened this issue Dec 15, 2020 · 7 comments · May be fixed by #387
Assignees
Labels
breaking change This could break existing code
Milestone

Comments

@fluidsonic
Copy link

The parameter names are very weird for day-to-day use:

LocalDateTime(year = 2021, monthNumber = 1, dayOfMonth = 1, hour = 12)

I don't care that month is a number. That's what we have a type system for.
OfMonth for day is also redundant as that's obvious from the context.

public constructor(year: Int, monthNumber: Int, dayOfMonth: Int, hour: Int, minute: Int, second: Int = 0, nanosecond: Int = 0)

@ilya-g
Copy link
Member

ilya-g commented Dec 16, 2020

These parameters are named this way, so that one can get back the values passed to the constructor from the same-named properties of the constructed object.

@fluidsonic
Copy link
Author

That property dayOfMonth should probably also simply be called day.

Kotlin tries to be pragmatic for the majority of use cases. The vast majority when working with dates is using the day of month. Everybody and their mother knows what a day refers to when dealing with a year/month/day date (+ time). dayOfWeek and dayOfYear is derived information and the latter used very rarely in my observation. dayOfMonth is overly specific. With auto-completion you also clearly see that there's day, dayOfWeek and dayOfYear so there's little room for error.

So, if Kotlin is a language optimized for general application development and the majority of use cases, day should be totally sufficient in both cases, constructor and property.

@fluidsonic
Copy link
Author

fluidsonic commented Dec 16, 2020

Btw, the compiler makes things more confusing. I've provided year, month and day all as Int and with natural names.
The compiler complains that I have to provide three Ints. But I did.

image
(I've also just created KT-43960 for that.)

I also think that the names being more natural overweighs the benefit of having monthNumber symmetric with the property. Nobody says "month number" in day-to-day life, not even a developer. As for dayOfMonth see my previous reply.

@dkhalanskyjb
Copy link
Contributor

dkhalanskyjb commented Jul 26, 2022

You make a good point about dayOfMonth. I agree, nobody will be confused about the meaning of LocalDateTime.day.

Regarding monthNumber, Month is a nice, well-typed, unambiguous thing. monthNumber: Int, on the other hand, is not. Unfortunately, many systems use zero-based numbering for months, even while using one-based numbering for days. JavaScript comes to mind:

new Date(2020, 1, 1)
Date Sat Feb 01 2020 00:00:00 GMT+0100 (Central European Standard Time)

So, it makes sense for LocalDate(Time)? to highlight an area that would give potentially misleading results by using a longer name. "monthNumber? What kind of a number? The usual kind, 1..12, or zero-based?" If we manage to provoke a question of this kind, we have won.

In your example (which, I assume, is simplified, but nonetheless), Month.DECEMBER is, I think, a better choice, which should be more obvious with a less prominent month number, like 5 or 10.

WDYT?

@ilya-g
Copy link
Member

ilya-g commented Jul 26, 2022

I believe that numbering months from zero is an abomination and should not be considered a normal practice that we take into account when naming the corresponding parameter/property.

@dkhalanskyjb
Copy link
Contributor

What is an abomination is using the day field to store the index of the week in the month (https://docs.microsoft.com/en-us/windows/win32/api/timezoneapi/ns-timezoneapi-time_zone_information, see the StandardDate section). I agree we shouldn't provide any support for that kind of abominations. Zero-based months are fine by comparison and quite ubiquitous: https://grep.app/search?q=JANUARY%20%3D%200 https://grep.app/search?q=%22JANUARY%22%3A%200&words=true APIs are messy and often done as an afterthought (producing such abominations), so data plumbing benefits from getting rid of raw numbers as soon as possible in favor of typed arguments.

@dkhalanskyjb dkhalanskyjb added the breaking change This could break existing code label Dec 1, 2023
@dkhalanskyjb
Copy link
Contributor

Here's the proposal:

  • Rename dayOfMonth to day everywhere. It's consistent with how we have LocalTime.nanosecond and don't call it nanosecondOfSecond (as opposed to nanosecondOfDay, for example).
  • Remove monthNumber from LocalDate: it's easy to access it as month.number, which also guides people to just use month. There's some code out there like Month(date.monthNumber), for example.
  • Rename the monthNumber constructor parameter to just month.
  • Keep monthNumber in DateTimeComponents (as it's significantly different from month there and has its own uses).
  • Keep monthNumber in the format builders, as having month + monthName is less clear than monthNumber + monthName.

@dkhalanskyjb dkhalanskyjb self-assigned this Apr 12, 2024
@dkhalanskyjb dkhalanskyjb added this to the 0.7.0 milestone Apr 12, 2024
dkhalanskyjb added a commit that referenced this issue Apr 19, 2024
`monthNumber` is still called that in the formatting facilities
to distinguish `monthNumber` from `monthName`, and also in
`DateTimeComponents` because `monthNumber` can contain
out-of-bounds data and is useful even aside from being a view of
`Month`.

Fixes #84

Blocked by an IDE bug:
<https://youtrack.jetbrains.com/issue/KTIJ-29647/ReplaceWith-of-properties-just-erases-code>
dkhalanskyjb added a commit that referenced this issue May 27, 2024
`monthNumber` is still called that in the formatting facilities
to distinguish `monthNumber` from `monthName`, and also in
`DateTimeComponents` because `monthNumber` can contain
out-of-bounds data and is useful even aside from being a view of
`Month`.

Fixes #84

Blocked by an IDE bug:
<https://youtrack.jetbrains.com/issue/KTIJ-29647/ReplaceWith-of-properties-just-erases-code>
dkhalanskyjb added a commit that referenced this issue May 27, 2024
`monthNumber` is still called that in the formatting facilities
to distinguish `monthNumber` from `monthName`, and also in
`DateTimeComponents` because `monthNumber` can contain
out-of-bounds data and is useful even aside from being a view of
`Month`.

Fixes #84

Blocked by an IDE bug:
<https://youtrack.jetbrains.com/issue/KTIJ-29647/ReplaceWith-of-properties-just-erases-code>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This could break existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@fluidsonic @ilya-g @dkhalanskyjb and others