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

Discuss whether DeclarationKind.RecordStruct should be shipped or deleted #52799

Closed
Youssef1313 opened this issue Apr 21, 2021 · 15 comments
Closed

Comments

@Youssef1313
Copy link
Member

Youssef1313 commented Apr 21, 2021

See #48136 (comment) for context.

cc @jcouv @sharwell @CyrusNajmabadi

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 21, 2021
@sharwell
Copy link
Member

sharwell commented Apr 21, 2021

It's my preference to delete RecordStruct, and mark RecordClass as:

[Obsolete($"This value is not used. Use {nameof(Class)} instead.")]
[EditorBrowsable(EditorBrowsableState.Never)]

@CyrusNajmabadi
Copy link
Member

I'm fine with that.

@jcouv
Copy link
Member

jcouv commented Apr 21, 2021

If any changes happen relative to records in the next week or two, please make the base the change off of the features/record-structs branch to avoid creating more conflicts.

@sharwell
Copy link
Member

@jcouv This is technically a change, but based on the PR it seems unlikely to conflict

@jcouv
Copy link
Member

jcouv commented Apr 21, 2021

I'm saying, why risk it? I've been dealing with conflicts for the last week or two and would like to avoid more. Thanks

@Youssef1313
Copy link
Member Author

@jcouv Would it help if I created two PRs for both main and record-structs branches with the same commit?

@jcouv
Copy link
Member

jcouv commented Apr 22, 2021

We try limiting duplicating changes. Is this important to fix in 16.9, or is this planned for 17.0? If the latter, then bundle with record-structs.
By the way, the record-structs branch added DeclarationKind.RecordStruct, so if we want to deprecate DeclarationKind.RecordClass... That is another reason to make the change in the record-structs (or soon features/compiler) branch.

@sharwell
Copy link
Member

This is a workspaces API and myself and @CyrusNajmabadi are in favor of #52803, and @tmat doesn't object to the change. I think we are ready to proceed and will make the team is aware of the pending change.

@sharwell
Copy link
Member

sharwell commented Apr 22, 2021

@jcouv Note that a conflict is unavoidable. We are updating main to correct the use of RecordClass, but we cannot automatically update (i.e. remove) RecordStruct. This field will need to be removed prior to or during the merge to the feature branch.

@jcouv
Copy link
Member

jcouv commented Apr 22, 2021

@sharwell The conflict is avoidable if the present change can be made in 17.0 instead of 16.9.

@Youssef1313
Copy link
Member Author

@jcouv PR #52803 was already merged. But it may not cause conflicts.
To remove RecordStruct I need main to be merged into features/compiler, is this something that will be done anytime soon?

@jcouv
Copy link
Member

jcouv commented Apr 27, 2021

@Youssef1313 I'll refresh the features/compiler branch today.

@jinujoseph jinujoseph added Concept-Continuous Improvement and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 27, 2021
@jinujoseph jinujoseph added this to the 17.0 milestone Apr 27, 2021
@jcouv
Copy link
Member

jcouv commented Apr 27, 2021

@Youssef1313 Here's the PR to refresh features/compiler branch: #52973

@Youssef1313
Copy link
Member Author

Thanks @jcouv. Opened #52988 to address this issue.

@Youssef1313
Copy link
Member Author

Fixed in #52988

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants