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: inline infallible CreateDataPropertyOrThrow calls #2528

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaelficarra
Copy link
Member

There's no reason to call CreateDataPropertyOrThrow when it cannot fail (marked with !). Instead, call CreateDataProperty directly.

Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

This no longer indicates to the reader that the property is guaranteed to be created, since CreateDataProperty can return false and there is nothing to indicate to the reader that it does not in these cases. I'm opposed.

@ljharb
Copy link
Member

ljharb commented Sep 21, 2021

Indeed, CreateDataProperty can return false. This caused many spec bugs in the past when authors didn't realize that this needed to be explicitly checked, which was why they were all explicitly converted to ! CreateDataPropertyOrThrow.

In this case, your change would require an assertion that the result was true, which is currently lacking in the PR.

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Oct 28, 2021
@ljharb ljharb force-pushed the frompropertydescriptor branch from 5af73df to 4a8b29d Compare October 28, 2021 13:56
@michaelficarra
Copy link
Member Author

Result from editor call today is that we should make a new AO for infallible CreateDataProperty and use that instead to make it more clear, but we'll wait to do that until after completion record reform is complete.

@ljharb
Copy link
Member

ljharb commented Nov 4, 2021

@michaelficarra should this be left open until then? or closed?

@michaelficarra
Copy link
Member Author

let's leave it open and do the work here

@bakkot bakkot removed the editor call to be discussed in the next editor call label Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants