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

Remove IFromIndexOperation completely #34306

Merged
merged 2 commits into from
Mar 26, 2019

Conversation

agocke
Copy link
Member

@agocke agocke commented Mar 20, 2019

We previously decided to remove IFromIndexOperation in favor of using an IUnaryOperation to represent the ^ syntax in C#, but did not have time to remove the interface completely. This PR finishes the change.

@agocke agocke marked this pull request as ready for review March 21, 2019 20:48
@agocke agocke requested review from a team as code owners March 21, 2019 20:48
@jcouv jcouv added this to the 16.1.P1 milestone Mar 21, 2019
/// <summary>
/// Represents the C# '^' operator.
/// </summary>
Hat = 0x7,
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 21, 2019

Choose a reason for hiding this comment

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

Hat [](start = 8, length = 3)

Is this how it will be called in the language spec? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Microsoft.CodeAnalysis.CSharp.LanguageVersion.LatestMajor = 2147483645 -> Microsoft.CodeAnalysis.CSharp.LanguageVersion
Microsoft.CodeAnalysis.CSharp.LanguageVersion.Preview = 2147483646 -> Microsoft.CodeAnalysis.CSharp.LanguageVersion
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there meaningful changes in this file? Please revert changes that simply move lines around

Copy link
Member Author

Choose a reason for hiding this comment

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

Every change to the PublicAPI files is supposed to be followed by a call to sort-unshipped.cmd. That's all I did here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you prefer I change that in a separate PR? Or as separate commit to make reviewing easier?

Copy link
Contributor

Choose a reason for hiding this comment

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

Every change to the PublicAPI files is supposed to be followed by a call to sort-unshipped.cmd.

This is the first time I hear about that. I see very little benefit and a lot of negatives in doing this. People should be able to examine the diff and quickly see what has changed, not just in PRs, but in history as well. Merge tools should be able to merge non-conflicting API changes automatically.

Would you prefer I change that in a separate PR? Or as separate commit to make reviewing easier?

I prefer that sorting was not done in this PR, or any other PR.


In reply to: 267972689 [](ancestors = 267972689)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorting is extremely helpful for resolving conflicts in merge PRs, as changes to the public API are extremely likely to be added in the same section without sorting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorting is extremely helpful for resolving conflicts in merge PRs, as changes to the public API are extremely likely to be added in the same section without sorting.

I have an opposite experience. Also, when one merges additions from different branches he should be able to take both sides and move on, without having to run some other tools to sort, etc.


In reply to: 268290574 [](ancestors = 268290574)

@@ -1,4 +1,3 @@
Microsoft.CodeAnalysis.Editing.DeclarationModifiers.WithIsRef(bool isRef) -> Microsoft.CodeAnalysis.Editing.DeclarationModifiers
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this change.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 21, 2019

Done with review pass (iteration 2) #Closed

@agocke agocke force-pushed the remove-ifromendindex-ioperation branch from 2f33ee7 to 116b5f9 Compare March 22, 2019 18:36
@agocke agocke force-pushed the remove-ifromendindex-ioperation branch from 116b5f9 to b42383a Compare March 26, 2019 17:26
@agocke
Copy link
Member Author

agocke commented Mar 26, 2019

@AlekseyTs I've removed the formatting change and just left the required changes.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 2)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 2)

@agocke agocke merged commit 2fbe5ad into dotnet:master Mar 26, 2019
@agocke agocke deleted the remove-ifromendindex-ioperation branch March 26, 2019 22:19
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.

5 participants