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

Editorial: Store ToString(𝔽(k)) in a variable for consistency #3531

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

yossydev
Copy link
Contributor

@yossydev yossydev commented Feb 4, 2025

ToString(𝔽(k)) was sometimes stored in let Pk and sometimes not. While this does not affect behavior, for consistency in the specification, I have standardized it to be stored in a variable.

Fixes #3530

@yossydev yossydev changed the title Editorial: For consistency, store ToString(𝔽(_k_)) in a variable befo… Editorial: Store ToString(𝔽(k)) in a variable for consistency Feb 4, 2025
@ljharb ljharb requested a review from a team February 4, 2025 01:49
@michaelficarra
Copy link
Member

I don't like doing this in places where the alias would only be referred to once. I am okay with indexOf and lastIndexOf because the alias is referred to twice in those AOs.

@yossydev yossydev force-pushed the chore/to-string-let-pl branch from 01df6e8 to ae16f29 Compare February 4, 2025 04:11
@yossydev
Copy link
Contributor Author

yossydev commented Feb 4, 2025

@michaelficarra

I don't like doing this in places where the alias would only be referred to once. I am okay with indexOf and lastIndexOf because the alias is referred to twice in those AOs.

Thank you!
I have only modified indexOf and lastIndexOf.

@yossydev yossydev force-pushed the chore/to-string-let-pl branch from ae16f29 to b2107f8 Compare February 4, 2025 04:14
@linusg
Copy link
Member

linusg commented Feb 4, 2025

Related: b1c00ce

@yossydev
Copy link
Contributor Author

yossydev commented Feb 5, 2025

Error: This request has been automatically failed because it uses a deprecated version of actions/upload-artifact: v3. Learn more: https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/

Would it be better to upgrade the version?

@bakkot
Copy link
Contributor

bakkot commented Feb 5, 2025

@yossydev That's happening elsewhere; don't worry about it here.

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Feb 5, 2025
ljharb pushed a commit to yossydev/ecma262 that referenced this pull request Feb 18, 2025
@ljharb ljharb force-pushed the chore/to-string-let-pl branch from b2107f8 to cbc6187 Compare February 18, 2025 17:31
@ljharb
Copy link
Member

ljharb commented Feb 18, 2025

@yossydev would you please register as a contributor?

@ljharb ljharb marked this pull request as draft February 18, 2025 18:25
ljharb pushed a commit to yossydev/ecma262 that referenced this pull request Feb 18, 2025
@ljharb ljharb force-pushed the chore/to-string-let-pl branch from cbc6187 to f2b9485 Compare February 18, 2025 18:29
@ljharb ljharb force-pushed the chore/to-string-let-pl branch from f2b9485 to 0f3de74 Compare February 18, 2025 18:52
@yossydev
Copy link
Contributor Author

@yossydev would you please register as a contributor?

@ljharb
I have registered!

@ljharb ljharb marked this pull request as ready for review February 19, 2025 00:03
@ljharb ljharb merged commit 0f3de74 into tc39:main Feb 19, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why do some TypedArray methods not use let Pk = ! ToString(𝔽(k)), unlike Array.prototype.indexOf?
5 participants