temporal icon indicating copy to clipboard operation
temporal copied to clipboard

Bikeshed for method naming conventions

Open nekevss opened this issue 1 year ago • 3 comments

There has been various changes in naming for methods over the lifetime of the development of temporal_rs, specifically around the use of snake case in methods and file names. The intended goal of this issue is to come to a consensus moving forward for naming conventions in temporal_rs.

So the first question to answer should probably be:

  1. Should snake case be adhered to 100% of the time regardless of method length?
  2. Should this naming convention also then be extended to filenames and modules?
  3. Should "Plain" be preserved in method names: "to_date" vs. to_plain_date or "to_time" vs. "to_plain_time"

General thoughts

My general thought on this subject is that naming in general is a question of to what degree snake case should be preserved in the general API. For instance, ZonedDateTime as a type is fine for a struct name, but can create some rather long and unyielding method names that may not make the best API if expanded to full snake case, e.g. to_zoned_date_time. In order to avoid long snake case methods that may make methods hard to deal with, ZonedDateTime has been condensed into one "proper" noun of sorts, e.g. to_zoneddatetime.

The issue with doing this type of naming is how it extends to other methods:

  • to_zoned_date_time vs. to_zoneddatetime
  • to_plain_date_time vs to_plaindatetime
  • to_plain_time vs to_plaintime

There are a few general names that have gone through various names and are in scope in connection to this issue (see below).

Time Zone

  • tz, timezone, or time_zone

Time zone has changed in various points.

PlainDateTime / DateTime

  • datetime, plaindatetime, date_time, plain_date_time

ZonedDateTime

  • zoneddatetime, zoned_date_time

General thoughts?

Also, if anyone has any other thoughts regarding to this issue that I may have missed feel free to let me know

nekevss avatar Jan 07 '25 04:01 nekevss

  • datetime, plaindatetime, date_time, plain_date_time

I'd suggest avoiding datetime nor date_time. Timezone-less date/time types are prone to bugs caused by programmers assuming that the offset will not change. By naming a zoneless type datetime it implies that the prefix-less type is the default type that should be used. In Temporal, we added the prefix "Plain" to make it clear that the developer needs to make a choice about what date/time type they want, rather than choosing a default-looking type.

justingrant avatar Mar 25 '25 19:03 justingrant

we added the prefix "Plain" to make it clear that the developer needs to make a choice about what date/time type they want, rather than choosing a default-looking type.

That's a good call out. The goal at this point, at least from my perspective, is to mimic the Temporal specification API as close as possible. So if there is a method called toPlainDateTime, then we would translate it to to_plain_datetime/to_plain_date_time.

This issue now that I think about it may a bit overloaded. Part of the issue with our naming convention is we have parameters and modules named various things that are not aligned (mostly due to shifting opinions on my part of what is probably better). So is it best to snake case EVERYTHING or only certain things.

For instance, the example above. Should we treat DateTime as a unit or separate, e.g. date_time vs. datetime. This then extends to other APIs. to_plaindatetime vs. to_plain_date_time vs. to_plain_datetime or to_zoneddatetime vs. to_zoned_date_time vs. to_zoned_datetime.

It's also worth noting that this principle would also be extended to module names as well. Currently our module for ZonedDateTime is zoneddatetime. But should what should that be updated to full snake case.

I know this is EXTREMELY semantic, but I'd ideally like to have one unified approach for temporal_rs. Was there ever any discussion regarding whether a PlainDateTime could be interpreted as a plain datetime vs. a plain date time?

nekevss avatar Mar 25 '25 19:03 nekevss

I know this is EXTREMELY semantic, but I'd ideally like to have one unified approach for temporal_rs. Was there ever any discussion regarding whether a PlainDateTime could be interpreted as a plain datetime vs. a plain date time?

Nope. Although you could infer from the PascalCased PlainDateTime that it would have been PlainDatetime if we'd considered "datetime" to be one word. So I'd assume that plain_date_time is probably OK for Rust.

justingrant avatar Mar 25 '25 20:03 justingrant