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

Update PublicAPI for 17.7 #69767

Merged
merged 3 commits into from
Aug 31, 2023
Merged

Conversation

ryzngard
Copy link
Contributor

No description provided.

@ryzngard ryzngard requested review from a team as code owners August 30, 2023 20:12
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 30, 2023
@ghost ghost added the Needs API Review Needs to be reviewed by the API review council label Aug 30, 2023
@ghost
Copy link

ghost commented Aug 30, 2023

This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging.

@ryzngard ryzngard requested a review from 333fred August 30, 2023 20:12
@@ -85,6 +93,7 @@ Microsoft.CodeAnalysis.CSharp.Conversion.Equals(Microsoft.CodeAnalysis.CSharp.Co
Microsoft.CodeAnalysis.CSharp.Conversion.Exists.get -> bool
Microsoft.CodeAnalysis.CSharp.Conversion.IsAnonymousFunction.get -> bool
Microsoft.CodeAnalysis.CSharp.Conversion.IsBoxing.get -> bool
Microsoft.CodeAnalysis.CSharp.Conversion.IsCollectionLiteral.get -> bool
Copy link
Member

Choose a reason for hiding this comment

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

@cston Just want to confirm we intend to keep this API name (as opposed to IsCollectionExpression or some such)

Copy link
Member

Choose a reason for hiding this comment

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

This property was renamed in 17.8 to IsCollectionExpression (see #69114).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For cases like this, do we just resolve when we ship the 17.8 as a remove/add?

Copy link
Member

Choose a reason for hiding this comment

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

@cston What do you think of keeping IsCollectionLiteral as unshipped in 17.7?

Copy link
Member

Choose a reason for hiding this comment

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

This probably needs to be renamed. Unless we also have IsTupleLiteral for the conversion as well.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think of keeping IsCollectionLiteral as unshipped in 17.7?

Does that mean, leave IsCollectionLiteral in Public.Unshipped.txt for 17.7? What would that affect?

Copy link
Member

Choose a reason for hiding this comment

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

You can also mark this as shipped but have a REMOVE line in the unshipped for 17.8. It makes it clearer that it would be a break in that case.

Copy link
Member

Choose a reason for hiding this comment

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

Either seems fine to me.
Since the collection expressions feature is only in preview in 17.7, it seems fine to leave this API as unshipped. But we can also add it and remove it later...

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave it as unshipped.

@jcouv
Copy link
Member

jcouv commented Aug 30, 2023

A couple of files show only whitespace/newline changes (LanguageServer/Protocol and RazorCompiler). Can we revert those?

@jcouv jcouv self-assigned this Aug 30, 2023
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.

Done with review pass (iteration 1)

@ryzngard
Copy link
Contributor Author

A couple of files show only whitespace/newline changes (LanguageServer/Protocol and RazorCompiler). Can we revert those?

A couple of files show only whitespace/newline changes (LanguageServer/Protocol and RazorCompiler). Can we revert those?

Done

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 2)

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)

@ryzngard ryzngard merged commit d7f0cdd into dotnet:release/dev17.7 Aug 31, 2023
25 checks passed
@ryzngard ryzngard deleted the mantis/17.7_shipped branch August 31, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Area-Infrastructure Needs API Review Needs to be reviewed by the API review council untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants