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: More changes for comparisons #3206

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Editorial: More changes for comparisons #3206

merged 1 commit into from
Feb 22, 2024

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Oct 20, 2023

... in line with PR #2877 and PR #3202.

The first commit changes:
_X_ and _Y_ are [not] the same {Realm,Module} Record
to just:
_X_ is [not] _Y_

The second commit changes:
SameValue(_X_, _Y_) is {*true*,*false*}
to
_X_ is [not] _Y_
when the two values are both Strings, or else one is a String and the other is a String or *null*.

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.

this WFM though I think one of the other editors preferred to keep "the same X Record" when we talked about this last

@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 23, 2023

... I think one of the other editors preferred to keep "the same X Record" when we talked about this last

It seemed odd to me that #2877 didn't eliminate that form, so I checked the PR's comments for any discussion of it, but didn't find anything.

@bakkot
Copy link
Contributor

bakkot commented Oct 23, 2023

That sounds like the sort of opinion I'd have. I do kind of prefer "the same X Record" because it makes it really, really clear that comparison is by identity rather than structural, without you having to keep in mind the details of how identity works for each kind of spec value.

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Oct 23, 2023
@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 25, 2023

it makes it really, really clear that comparison is by identity rather than structural,

So are there other kinds of values for which you'd like this clarity?

@bakkot
Copy link
Contributor

bakkot commented Oct 25, 2023

Only really Records and Lists.

@bakkot
Copy link
Contributor

bakkot commented Nov 1, 2023

Per call, we mildly prefer to keep the "same Record as" form, extending I guess to using it in the existing Repeat, while _thisEnv_ is not _varEnv_ case.

@bakkot
Copy link
Contributor

bakkot commented Nov 1, 2023

But we do like the change to use is instead of SameValue for comparing strings.

@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Nov 1, 2023
@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 7, 2023

Okay, I've dropped the first commit (reinstating the status quo uses of are the same Foo Record), and I've added some more commits to align the spec with comparison conventions.

jmdyck added a commit to jmdyck/ecmaspeak-py that referenced this pull request Nov 15, 2023
@syg syg added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Feb 21, 2024
@jmdyck
Copy link
Collaborator Author

jmdyck commented Feb 21, 2024

(force-pushed to squash it down to one commit)

Specifically:
- change `SameValue` to `is` for comparison of String values
- change `SameValue` to `is` for comparison of Number values
- change `is` to `=` for comparison of BigInts
- change `is` to `=` for comparison of math values
- change `is` to `are the same X Record` for comparison of Records
@ljharb ljharb merged commit f5529ca into tc39:main Feb 22, 2024
7 checks passed
@jmdyck jmdyck deleted the SameValue branch February 25, 2024 03:03
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.

None yet

5 participants