icu4x icon indicating copy to clipboard operation
icu4x copied to clipboard

Consider naming of nested types in icu::datetime

Open sffc opened this issue 3 years ago • 1 comments

Feedback from @markusicu:

In the example in resolve_components, the expected_components_bag fields are year, month, day, but it might make more readable to be year_style, month_length, etc., because the RHS values are not the numeric values for the year, month, & day, but rather styles. And maybe the type names should not just be “Year”, but “YearStyle”, etc., so that when you import the types (structs, enums, etc.) into your code, you don’t get misleading code from the POV of a new user.

It’s strange that time_zone::IsoMinutes is an enum rather than a number type. More often, we have called such a thing a “-Style”.

For datetime::fields::*, I would rename “Day” to “DayType” or “DayField”

The structs in datetime::input::* are actually what they say they are, so they wouldn’t need a rename

@zbraniecki

sffc avatar Sep 03 '22 01:09 sffc

  • @zbraniecki - There's a large number of small types in this module. You should import the module, like fields, and then everywhere you use it, you write fields::Year, etc. I think it reads better than importing YearType, MonthType, etc., over and over. So, as the author, it's intentional. But, it's naming, so it's subjective, and I can see the opposite angle. Rather than focusing on the names, I would focus on what is easiest to use and write code.
  • @robertbastian - I think the way @zbraniecki did it makes sense.
  • @sffc - I favor keeping the status quo unless there's a compelling reason to switch. This is clearly in the gray area with no clear right answer, so maybe not worth the switch.
  • @robertbastian - The style guide suggests to export named types.
  • @zbraniecki - I think that section of the style guide is for a module that has a small number of types that are imported on their own. But here, we have a large number of types, all inside a single module, so people should import the module. I think the style guide should reflect this.
  • @robertbastian - I wonder if there's a lint for this: tagging exported items as intended to be imported by their module (always qualified).
  • @Manishearth - This pattern is relatively rare, although idiomatic. People don't typically like to import modules, but it's okay to do in specific circumstances.

Recommended actions:

  • [ ] Update the style guide
  • [ ] Ensure docs examples use the module-style imports

LGTM: @robertbastian @sffc @zbraniecki @Manishearth

sffc avatar Sep 08 '22 17:09 sffc