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

Normative: Change the getters to methods #65

Closed
wants to merge 4 commits into from
Closed

Conversation

FrankYFTang
Copy link
Collaborator

Close #62

Per 2022-11-30 TC39 recommentations

@FrankYFTang
Copy link
Collaborator Author

@bathos @rkirsling

@FrankYFTang
Copy link
Collaborator Author

@anba

@FrankYFTang
Copy link
Collaborator Author

We need to change the following test262 tests with this PR

'intl402/Locale/prototype/calendars/branding': [FAIL],
'intl402/Locale/prototype/calendars/name': [FAIL],
'intl402/Locale/prototype/calendars/output-array': [FAIL],
'intl402/Locale/prototype/calendars/prop-desc': [FAIL],
'intl402/Locale/prototype/collations/branding': [FAIL],
'intl402/Locale/prototype/collations/name': [FAIL],
'intl402/Locale/prototype/collations/output-array': [FAIL],
'intl402/Locale/prototype/collations/output-array-values': [FAIL],
'intl402/Locale/prototype/collations/prop-desc': [FAIL],
'intl402/Locale/prototype/hourCycles/branding': [FAIL],
'intl402/Locale/prototype/hourCycles/name': [FAIL],
'intl402/Locale/prototype/hourCycles/output-array': [FAIL],
'intl402/Locale/prototype/hourCycles/output-array-values': [FAIL],
'intl402/Locale/prototype/hourCycles/prop-desc': [FAIL],
'intl402/Locale/prototype/numberingSystems/branding': [FAIL],
'intl402/Locale/prototype/numberingSystems/name': [FAIL],
'intl402/Locale/prototype/numberingSystems/output-array': [FAIL],
'intl402/Locale/prototype/numberingSystems/prop-desc': [FAIL],
'intl402/Locale/prototype/textInfo/branding': [FAIL],
'intl402/Locale/prototype/textInfo/name': [FAIL],
'intl402/Locale/prototype/textInfo/output-object': [FAIL],
'intl402/Locale/prototype/textInfo/output-object-keys': [FAIL],
'intl402/Locale/prototype/textInfo/prop-desc': [FAIL],
'intl402/Locale/prototype/timeZones/branding': [FAIL],
'intl402/Locale/prototype/timeZones/name': [FAIL],
'intl402/Locale/prototype/timeZones/output-array': [FAIL],
'intl402/Locale/prototype/timeZones/output-array-sorted': [FAIL],
'intl402/Locale/prototype/timeZones/output-undefined': [FAIL],
'intl402/Locale/prototype/timeZones/prop-desc': [FAIL],
'intl402/Locale/prototype/weekInfo/branding': [FAIL],
'intl402/Locale/prototype/weekInfo/name': [FAIL],
'intl402/Locale/prototype/weekInfo/output-object': [FAIL],
'intl402/Locale/prototype/weekInfo/output-object-keys': [FAIL],
'intl402/Locale/prototype/weekInfo/prop-desc': [FAIL],

@FrankYFTang
Copy link
Collaborator Author

tc39/test262#3746

webkit-early-warning-system pushed a commit to Constellation/WebKit that referenced this pull request Dec 7, 2022
https://bugs.webkit.org/show_bug.cgi?id=248836
rdar://problem/103041601

Reviewed by Justin Michaud.

Nov 30 TC39 meeting decided to change Intl.Locale info getters to methods[1] because of object identity issue[2].
This patch aligns our implementation to this change.

[1]: tc39/proposal-intl-locale-info#65
[2]: tc39/proposal-intl-locale-info#62

* JSTests/stress/intl-locale-info.js:
(throw.new.Error):
(shouldBe):
(let.l.new.Intl.Locale.shouldBe):
* JSTests/test262/expectations.yaml:
* Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

Canonical link: https://commits.webkit.org/257449@main
@gibson042
Copy link

gibson042 commented Dec 8, 2022

My approval stands, but I would also like to see the names of properties that users must invoke as functions indicate that fact (and align with precedent such as Date.prototype) with a "get" prefix, especially since Locale instances already have non-function properties and it will definitely confuse practitioners if e.g. getting desired data looks like const weekInfo = locale.weekInfo(); (parentheses must be present) and const calendarName = locale.calendar; (parentheses must not be present) with no hint about how to differentiate the two cases.

  • calendarsgetCalendars
  • collationsgetCollations
  • hourCyclesgetHourCycles
  • numberingSystemsgetNumberingSystems
  • timeZonesgetTimeZones
  • textInfogetTextInfo
  • weekInfogetWeekInfo

@sffc
Copy link
Contributor

sffc commented Dec 8, 2022

TG2 discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2022-12-08.md#normative-change-the-getters-to-methods-65

Conclusion: We are not clear on whether the functions should have a get prefix, like getTextInfo() or getCollations() versus textInto() or collations(). We would like to appeal for further discussion.

CC @zbraniecki @bakkot @michaelficarra @waldemarhorwat @ljharb @ptomato

@bathos
Copy link

bathos commented Dec 9, 2022

Soft preference for the get prefix because I find it adds considerable clarity when reading and likely aids discovery & conveys the expected usage pattern. You don’t need to look up whether or not getCalendars is a method.

(Arguing against myself here, but:) because Intl already omits it for the Intl.<constructor>.prototype.resolvedOptions methods and web platform API norms seem to be converging on omission of get in similar cases too (e.g. HTMLSlotElement.prototype.assignedNodes), a consistency argument could favor omission. The Date.prototype.getFoo methods seem more like pre-accessor oddballs to me than a strong precedent: they return primitives and each has a corresponding setFoo. That particular pattern would be weird in a new API except when additional arguments are needed or could make sense (e.g. DataView).

@gibson042
Copy link

gibson042 commented Dec 9, 2022

Soft preference for the get prefix because I find it adds considerable clarity when reading and likely aids discovery & conveys the expected usage pattern. You don’t need to look up whether or not getCalendars is a method.

Exactly (elaborated further above).

That particular pattern would be weird in a new API except when additional arguments are needed or could make sense (e.g. DataView).

Or when some properties are data getters while others must be invoked as methods, which is the case here. It's true that Map and Set already have that mix with a non-method size, but a) it's much easier to remember a single-property exception than a big jumble of both kinds, and b) even so, that kind of confusion does exist—people (including me) really do write map.size() before realizing their mistake, and we should strive to limit such friction.

@zbraniecki
Copy link
Member

My position is soft rejection of get prefix - new APIs such as Map and Set do not use get* in cases such as Map::keys(), Map::entries(), Map::values() and I enjoy Rust code convention discouraging from use of get for getters.

@ljharb
Copy link
Member

ljharb commented Dec 10, 2022

Those are all actual getters - methods that get are nice with the “get” prefix, since they’re a verb, which indicates function-ness.

@zbraniecki
Copy link
Member

@ljharb I'm confused by your response. Can you rephrase please?

@ljharb
Copy link
Member

ljharb commented Dec 10, 2022

@zbraniecki ”actual getters” are seemingly normal properties that magically/implicitly call a function - as such, it’s appropriate to name them as if they were properties. Things that look like functions should generally be named with a verb, since they do things - and in this case, the thing the methods do is “get” something.

You’re totally right that keys/entries/values aren’t actual getters - my mistake - but i don’t think those APIs are super clearly named (nor is it new in ES6; Object.keys eg is in ES5)

@zbraniecki
Copy link
Member

Thanks, I'm still a bit confused about your position here.

”actual getters” are seemingly normal properties that magically/implicitly call a function - as such, it’s appropriate to name them as if they were properties. Things that look like functions should generally be named with a verb, since they do things - and in this case, the thing the methods do is “get” something.

keys is "actual getter" according to that logic, or "thing that looks like function"? What about "entries", "values", "collations", "calendars", "timeZones"?

Which ones, in your opinion, are "actual getters" and which ones are "things that look like functions"?

@ljharb
Copy link
Member

ljharb commented Dec 10, 2022

When i say “actual” I’m referring to whether it’s an accessor property vs a function-valued data property.

A function looks like a function when you invoke it with parens.

@zbraniecki
Copy link
Member

When i say “actual” I’m referring to whether it’s an accessor property vs a function-valued data property.

Can you classify using your mental model the following:

  • Map::keys,
  • Map::entries,
  • Map::values,
  • LocaleInfo::collations,
  • LocaleInfo::calendars,
  • LocaleInfo::timeZones

?

@ljharb
Copy link
Member

ljharb commented Dec 10, 2022

I'm not familiar with the LocaleInfo ones, but yes, Map/Array/Set keys/entries/values are methods, but poorly named imo since they aren't verbs - they're named more like accessors (but their semantics aren't appropriate for accessors, since they return a new object every time). I don't want to repeat that pattern elsewhere if we can avoid it.

@zbraniecki
Copy link
Member

Thanks, so I derive from your words that your position is that Map::entries() should be Map::getEntries() etc. And therefore analogous methods on LocaleInfo should also use get*.

I don't want to repeat that pattern elsewhere if we can avoid it.

Do you feel this position is universally accepted in TC39, is the group split, or do you think your opinion is in minority?

@ljharb
Copy link
Member

ljharb commented Dec 10, 2022

Yep, that sounds accurate.

I don't think this kind of naming question has come up before such that I have any idea what the group thinks - the naming of Object.values/entries was precedented by Object.keys, and nothing similar that I can recall has been added since ES6 (ie, with the modern process).

@bathos
Copy link

bathos commented Dec 11, 2022

In addition to aligning with the static Object methods (for which “methodness” is relatively unambiguous), the keys/entries/values trio constitutes an informal generic iterable collection contract shared by Array, Map and Set. On the web platform, these are joined by each interface that uses iterable, maplike, and setlike declarations, too. That generality & ubiquity impacts the calculus a bit imo? There’s nothing similarly generic about concepts like “the calendars of something”.

@zbraniecki
Copy link
Member

There’s nothing similarly generic about concepts like “the calendars of something”.

There isn't indeed. But I'm struggling a mental model that says "if a method is to be shared between a range of types, then it shouldn't use the prefix, but if it's just for one type, it should".

Such logic doesn't scale as the API evolves and one doesn't know how many types will reuse it when we introduce it for the first type.
One can try to make such claims about LocaleInfo::calendars, but I wouldn't be surprised if a couple years later some proposal were to reuse calendars and use LocaleInfo::calendars as a precedence, and so on.

My position is the one derived from Rust code convention - getters shouldn't use get :) And I think I'm quite universal about it. My take is that get* method should be used to indicate a non-trivial dynamic operation of retrieval of a value - getElementById, getElementsByTagName, etc.

For all other cases, universally, I believe that any getter/setter pair should be ::x() and ::setX() and that includes all getters on LocaleInfo, since they do not require extensive computations, and are basically properties, but because in JS A::property !== A::property we want to use method to make it less confusing.
So we're workarounding JS limitation, but in spirit, it's a property.

@bathos
Copy link

bathos commented Dec 11, 2022

But I'm struggling a mental model that says "if a method is to be shared between a range of types, then it shouldn't use the prefix, but if it's just for one type, it should".

I agree — like ljharb had said, these examples had a particular “lineage”. I meant more that they happened to be less problematic.

My position is the one derived from Rust code convention - getters shouldn't use get :) And I think I'm quite universal about it.

Mine comes from JS + JS APIs on the web platform, where getters (in the sense of accessor property [[Get]] functions) are typically “noun-named” and methods are typically “verb-named” if they appear on meaningfully stateful objects. But there are enough exceptions that I couldn’t claim this is universal or anything.

For all other cases, universally, I believe that any getter/setter pair should be ::x() and ::setX()

As far as I know this would be a novel pattern for JS. It would seem very surprising to me. Where there is setX and X is gettable, it’s companion has been getX.

@zbraniecki
Copy link
Member

As far as I know this would be a novel pattern for JS. It would seem very surprising to me. Where there is setX and X is gettable, it’s companion has been getX.

How can you predict that? It's quite common in my experience that you start with a getter and only later extend the API to have a setter. If at the time of deciding on the getter you don't know if you'll ever need a setter (this is the case here - we don't think we'll need setters, but maybe in a couple years someone will propose one?) then as API designer you're forced to see the future.

Unless you want to say "not adding get* is appropriate in cases where the API designer can reasonably claim that setter for that property will never be needed". I can support such mental model.

In this case, we could ask ourselves - do we see any possibility that a setter for those will be needed, and if the answer is "no", then we would go without "get" prefix. Otherwise we'd use "get" prefix to prepare for potential "set". But this feels still fragile API naming convention.

@bathos
Copy link

bathos commented Dec 11, 2022

How can you predict that? It's quite common in my experience that you start with a getter and only later extend the API to have a setter. If at the time of deciding on the getter you don't know if you'll ever need a setter (this is the case here - we don't think we'll need setters, but maybe in a couple years someone will propose one?) then as API designer you're forced to see the future.

I think this is one reason why they’re almost always either real getters or, if something prevents that like here, methods that use getX naming? In both cases, you don’t have to predict that.

FWIW, in the web platform in particular, this sort of naming issue just doesn’t seem to show up much in the first place because actual getters outnumber “getX methods” by at least an order of magnitude. Web IDL “attributes” (accessors in the JS binding) that return objects are backed by internal state so that the same object is returned. I think the case for dropping get is weakened by the ubiquity of actual getters in both JS host implementations and user code, but more importantly, as gibson042 pointed out, there are some living on the same prototype being defined by this proposal. But I can see why the pattern you’ve described would make a lot of sense in languages that don’t also have an equivalent to accessor properties (since there’d be no ambiguity, right?)

@gibson042
Copy link

Even if there were general guidance against a "get" prefix, I would still argue for having it here to improve the developer experience by more clearly differentiating which properties of a Locale instance must be invoked as methods.

@FrankYFTang
Copy link
Collaborator Author

here is what I will do. I will keep this PR unchanged with the current name, but I will open another PR which have the name changed. So we will have two PR side by side and we will talk about in TC39 and I assume one of that will be picked (I assume the new one will be) . How about that?

@FrankYFTang
Copy link
Collaborator Author

Another PR with also the name changes in #67

@FrankYFTang
Copy link
Collaborator Author

In 2023-01-31 TC39 meeting the concensus is merge PR 67 but not PR65

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

Successfully merging this pull request may close these issues.

The textInfo getter seems unwell
6 participants