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

Rename record equality parameters to 'left' and 'right' #51973

Merged
merged 3 commits into from
Mar 20, 2021
Merged

Rename record equality parameters to 'left' and 'right' #51973

merged 3 commits into from
Mar 20, 2021

Conversation

C-xC-c
Copy link
Contributor

@C-xC-c C-xC-c commented Mar 18, 2021

Fixes #49300

@C-xC-c C-xC-c requested a review from a team as a code owner March 18, 2021 22:25
@dnfadmin
Copy link

dnfadmin commented Mar 18, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

@RikkiGibson RikkiGibson added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Mar 18, 2021
@RikkiGibson RikkiGibson self-assigned this Mar 18, 2021
@jcouv
Copy link
Member

jcouv commented Mar 18, 2021

In general, changing parameter names would be a breaking change since callers may use explicit argument names. But I think we're okay because those are operators.

We'd need a corresponding update to the spec:
https://github.com/dotnet/csharplang/blob/main/proposals/csharp-9.0/records.md#equality-members

@jcouv jcouv self-assigned this Mar 18, 2021
@C-xC-c
Copy link
Contributor Author

C-xC-c commented Mar 18, 2021

I have created a pull request for the required spec changes here dotnet/csharplang#4555

@C-xC-c
Update RecordTests.NominalRecordMembers
14c2798 
To reflect changes to record equality operators
@C-xC-c C-xC-c mentioned this pull request Mar 18, 2021
Merged
jcouv
jcouv approved these changes Mar 19, 2021
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 3)

@RikkiGibson
Copy link
Contributor

Planning to merge this afternoon, once we're sure that there's no objection from the language design team.

@RikkiGibson RikkiGibson merged commit 791d60f into dotnet:main Mar 20, 2021
@ghost ghost added this to the Next milestone Mar 20, 2021
@RikkiGibson
Copy link
Contributor

Thanks for your contribution @C-xC-c!

@Youssef1313 Youssef1313 mentioned this pull request Mar 20, 2021
Merged
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@RikkiGibson RikkiGibson

@jcouv jcouv

Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. New Language Feature - Records Records
Projects
None yet
Milestone
16.10.P2
Development

Successfully merging this pull request may close these issues.

Record operator parameter names are 'r1' and 'r2' rather than 'left' and 'right'
6 participants
@C-xC-c @dnfadmin @jcouv @RikkiGibson @cston @allisonchou