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.

Sign up for GitHub

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

Jump to bottom

Editorial: CreateListFromArrayLike: change element type list to a 2-state enum #3409

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Editorial: CreateListFromArrayLike: change element type list to a 2-state enum #3409

merged 1 commit into from
Sep 5, 2024

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Aug 28, 2024 •
edited
Loading

Per editor call discussion.

Related: #3339.

@ljharb ljharb requested a review from a team August 28, 2024 22:16
@ljharb ljharb mentioned this pull request Aug 28, 2024
Closed
ljharb added a commit to ljharb/ecma262 that referenced this pull request Aug 28, 2024
@ljharb
Editorial: CreateListFromArrayLike: change element type list to a 2…
f520124 
…-state enum (tc39#3409)
ljharb added a commit to ljharb/ecma262 that referenced this pull request Aug 28, 2024
@ljharb
Editorial: CreateListFromArrayLike: change element type list to a 2…
2df5e43 
…-state enum (tc39#3409)
@bakkot
Copy link
Contributor

bakkot commented Aug 28, 2024 •
edited
Loading

Temporal calls this AO with Object, so at minimum it cannot be used without introducing a third state. As I said in the other thread, I think listing the various special cases makes this more verbose than just being general, and I would mildly prefer to just keep the general form.

@ljharb
Copy link
Member Author

ljharb commented Aug 28, 2024

True - we'd need ~object~ to cover that use case.

I'll leave this as a draft until the three of you can come to a decision, but the hope was that this PR would help get us to complete removal of Type().

@ljharb ljharb marked this pull request as draft August 28, 2024 22:32
@ljharb ljharb added the editor call to be discussed in the next editor call label Aug 28, 2024
@anba
Copy link
Contributor

anba commented Aug 28, 2024

The Temporal caller to CreateListFromArrayLike will be removed in tc39/proposal-temporal#2925.

michaelficarra
michaelficarra reviewed Aug 28, 2024
View reviewed changes
spec.html Outdated Show resolved Hide resolved
michaelficarra
michaelficarra approved these changes Aug 28, 2024
View reviewed changes
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise.

ljharb added a commit to ljharb/ecma262 that referenced this pull request Aug 29, 2024
@ljharb
Editorial: CreateListFromArrayLike: change element type list to a 2…
1e06d09 
…-state enum (tc39#3409)
jmdyck
jmdyck reviewed Aug 29, 2024
View reviewed changes
spec.html Outdated Show resolved Hide resolved
ljharb added a commit to ljharb/ecma262 that referenced this pull request Aug 29, 2024
@ljharb
Editorial: CreateListFromArrayLike: change element type list to a 2…
bc0e504 
…-state enum (tc39#3409)
syg
syg approved these changes Sep 5, 2024
View reviewed changes
spec.html Outdated
): either a normal completion containing a List of ECMAScript language values or a throw completion
</h1>
<dl class="header">
<dt>description</dt>
<dd>It is used to create a List value whose elements are provided by the indexed properties of _obj_. _elementTypes_ contains the names of ECMAScript Language Types that are allowed for element values of the List that is created.</dd>
</dl>
<emu-alg>
1. If _elementTypes_ is not present, set _elementTypes_ to « Undefined, Null, Boolean, String, Symbol, Number, BigInt, Object ».
1. If _elementTypes_ is not present, set _elementTypes_ to ~all~.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. If _elementTypes_ is not present, set _elementTypes_ to ~all~.
1. If _validElementTypes_ is not present, set _validElementTypes_ to ~all~.

@syg
Copy link
Contributor

syg commented Sep 5, 2024

Temporal calls this AO with Object, so at minimum it cannot be used without introducing a third state. As I said #3339 (comment), I think listing the various special cases makes this more verbose than just being general, and I would mildly prefer to just keep the general form.

Since that use case is being removed, and since this AO has long existed and we didn't see much general use of it, so seems fine to me to not make it general.

michaelficarra
michaelficarra reviewed Sep 5, 2024
View reviewed changes
spec.html Outdated
@@ -6447,23 +6447,23 @@ <h1>
<h1>
CreateListFromArrayLike (
_obj_: an ECMAScript language value,
optional _elementTypes_: a List of names of ECMAScript Language Types,
optional _elementTypes_: ~all~ or ~property-key~,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameter actually gains nothing from being an enum. Though an enum is usually better, a boolean validatePropertyKeys or something would be just as clear here.

@ljharb ljharb removed the editor call to be discussed in the next editor call label Sep 5, 2024
ljharb added a commit to ljharb/ecma262 that referenced this pull request Sep 5, 2024
@ljharb
Editorial: CreateListFromArrayLike: change element type list to a 2…
64eded3 
…-state enum (tc39#3409)
@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Sep 5, 2024
@ljharb ljharb marked this pull request as ready for review September 5, 2024 22:30
@ljharb ljharb merged commit 6d6515a into tc39:main Sep 5, 2024
8 checks passed
@ljharb ljharb deleted the CreateListFromArrayLike branch September 5, 2024 22:54
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Oct 19, 2024
@jmdyck
Editorial: Fix CreateListFromArrayLike preamble
69d2ef7 
PR tc39#3409 changed a parameter of CreateListFromArrayLike,
but didn't make the corresponding change to the preamble.
(And PR tc39#3419 failed to fix it.)
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Oct 19, 2024
@jmdyck
Editorial: Fix CreateListFromArrayLike preamble
e464cb4 
PR tc39#3409 changed a parameter of CreateListFromArrayLike,
but didn't make the corresponding change to the preamble.
(And PR tc39#3419 failed to fix it.)
michaelficarra pushed a commit to jmdyck/ecma262 that referenced this pull request Oct 21, 2024
@jmdyck @michaelficarra
Editorial: Fix CreateListFromArrayLike preamble
9ab7664 
PR tc39#3409 changed a parameter of CreateListFromArrayLike,
but didn't make the corresponding change to the preamble.
(And PR tc39#3419 failed to fix it.)
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Nov 8, 2024
@jmdyck @ljharb
Editorial: Fix CreateListFromArrayLike preamble (tc39#3454)
e887590 
PR tc39#3409 changed a parameter of CreateListFromArrayLike,
but didn't make the corresponding change to the preamble.
(And PR tc39#3419 failed to fix it.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmdyck jmdyck jmdyck left review comments

@michaelficarra michaelficarra michaelficarra approved these changes

@syg syg syg approved these changes

Assignees
No one assigned
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ljharb @bakkot @anba @syg @michaelficarra @jmdyck