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

Change some metadata extension methods into DIM properties and annotate for nullability #23361

Merged
1 commit merged into from
Nov 18, 2020

Conversation

roji
Copy link
Member

@roji roji commented Nov 17, 2020

@AndriySvyryd we can also move other extension methods to be properties on the interfaces if we want (especially internal ones).

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Nov 17, 2020

All public extensions should continue to be methods to avoid the breaking change, using properties doesn't give any significant enough advantage. Eventually we'll need to convert all of them to DIM - #19213

@roji
Copy link
Member Author

roji commented Nov 17, 2020

@AndriySvyryd are you saying to want me to add back the extension methods, calling the new properties, to maintain binary backwards compat here? Or that you're against the properties altogether (although we want to do #19213 anyway)?

Re the advantage of properties, the main reason for introducing them was to be able to use the new [MemberNotNullWhen], which isn't usable on extension methods.

@AndriySvyryd
Copy link
Member

@roji No, I'm saying that they should be methods with DIM. We don't necessarily need binary compat, but we should preserve compile compat.

@roji
Copy link
Member Author

roji commented Nov 17, 2020

Will change.

Copy link
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

HasClrType can become a property, since it was pubternal before

@smitpatel smitpatel force-pushed the feature/tfm branch 3 times, most recently from cd77d47 to 2a11111 Compare November 18, 2020 03:25
Base automatically changed from feature/tfm to main November 18, 2020 04:15
@ghost
Copy link

ghost commented Nov 18, 2020

Hello @roji!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@smitpatel
Copy link
Member

(Removed auto-merge, I will add it back. Trying to get release/5.0 merged to main)

@AndriySvyryd
Copy link
Member

(nit) Commit description is not accurate

* Turn HasDefiningNavigation from extension to DIM
* Turn HasClrType into a DIM property
@roji
Copy link
Member Author

roji commented Nov 18, 2020

@AndriySvyryd fixed, @smitpatel no problem.

@ghost ghost merged commit 624781d into main Nov 18, 2020
@ghost ghost deleted the MemberNotNullWhen branch November 18, 2020 08:16
This pull request was closed.
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