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: Simplify AssignmentTargetType values. #1652

Open
wants to merge 1 commit into
base: master
from

Conversation

@rkirsling
Copy link
Member

commented Aug 1, 2019

Resolves #1643.

As expressed there, boolean seems best if nonsimple is the best enum value we can come up with. I've also taken the opportunity to simplify the name to IsSimpleAssignmentTarget since I'm not sure that "valid" really serves to clarify anything (but feel free to disagree).

@rkirsling rkirsling force-pushed the rkirsling:resimplify-assignmenttargettype branch 2 times, most recently from 11494a9 to 8c9a5eb Aug 1, 2019

@leobalter

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

cc @DanielRosenwasser this affects the optional chaining proposal which still uses IsValidSimpleAssignmentTarget.

Thanks for this PR, @rkirsling!

@DanielRosenwasser

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Thanks for the heads up! I'll change the proposals and existing PRs to reflect the appropriate changes.

@ljharb
Copy link
Member

left a comment

Per #1643 (comment), I am against using a boolean here. We had to expand from 2 to 3 values once, we will thus likely have to again, so an enum is most appropriate to me.

@rkirsling

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@ljharb That's fine, but you haven't counter-suggested what the false value should be called. 😕

@ljharb

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

The op's called "IsSimpleAssignmentTarget" - "simple" vs "nonsimple" or "complex"?

Alternatively we could rename the abstract operation itself but that'd be a harder bikeshed.

@rkirsling rkirsling force-pushed the rkirsling:resimplify-assignmenttargettype branch from 8c9a5eb to 1c9caa5 Aug 1, 2019

@rkirsling rkirsling changed the title Editorial: Change AssignmentTargetType back to a boolean. Editorial: Simplify AssignmentTargetType values. Aug 1, 2019

@rkirsling

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

Oh hmm, apparently the return values for GetGeneratorKind include ~non-generator~, so I guess that could be a precedent for ~non-simple~. Anybody have a strong preference?

@rkirsling

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

Changed to ~non-simple~ after all. I really think this is the best we can do given that the opposite of simple is either complex or invalid (and we don't actually have a reason to distinguish among those).

@rkirsling rkirsling force-pushed the rkirsling:resimplify-assignmenttargettype branch from 112e967 to e71d854 Aug 2, 2019

@rkirsling

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

Switched to @zenparsing's suggestion. Perhaps there's no need to overthink it.

@ljharb

ljharb approved these changes Aug 9, 2019

@ljharb ljharb requested review from zenparsing and tc39/ecma262-editors Aug 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.