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

Merge Features/quick info party into master-vs-deps #26003

Merged
merged 104 commits into from
Apr 24, 2018

Conversation

JieCarolHu
Copy link
Contributor

@JieCarolHu JieCarolHu commented Apr 6, 2018

Merge Features/quick info party into master-vs-deps

Customer scenario

The changes are already reviewed in the following PRs
#20554
#25181
#25484
#25975

Bugs this fixes

#24094
#24956
#24434
#25608

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

mattwar and others added 30 commits August 11, 2016 15:58
@JieCarolHu JieCarolHu requested review from a team as code owners April 6, 2018 20:44
Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

It seems like features/quick-info-party is not up-to-date with the latest from master-vs-deps. The changes to readme.md and tests.sh looks especially suspicious.

README.md Outdated
@@ -7,17 +7,15 @@
|:--:|:--:|:--:|:--:|:--:|:--:|:--:|:--:|
|**master**|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_unit32/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_unit32/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_unit64/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_unit64/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_release_unit32/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_release_unit32/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_release_unit64/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_release_unit64/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_determinism/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_determinism/)|[![Build Status](https://ci.dot.net/buildStatus/icon?job=dotnet_roslyn/master/windows_debug_vs-integration)](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_vs-integration/)|[![Build Status](https://ci.dot.net/buildStatus/icon?job=dotnet_roslyn/master/windows_release_vs-integration)](https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_release_vs-integration/)|
|**dev15.0.x**|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_debug_unit32/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_debug_unit32/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_debug_unit64/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_debug_unit64/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_release_unit32/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_release_unit32/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_release_unit64/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_release_unit64/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_determinism/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_determinism/)|[![Build Status](https://ci.dot.net/buildStatus/icon?job=dotnet_roslyn/dev15.0.x/windows_debug_vs-integration)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_debug_vs-integration/)|[![Build Status](https://ci.dot.net/buildStatus/icon?job=dotnet_roslyn/dev15.0.x/windows_release_vs-integration)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.0.x/job/windows_release_vs-integration/)|
|**dev15.5.x**|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_debug_unit32/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_debug_unit32/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_debug_unit64/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_debug_unit64/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_release_unit32/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_release_unit32/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_release_unit64/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_release_unit64/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_determinism/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_determinism/)|[![Build Status](https://ci.dot.net/buildStatus/icon?job=dotnet_roslyn/dev15.5.x/windows_debug_vs-integration)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_debug_vs-integration/)|[![Build Status](https://ci.dot.net/buildStatus/icon?job=dotnet_roslyn/dev15.5.x/windows_release_vs-integration)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.5.x/job/windows_release_vs-integration/)|
|**dev15.6.x**|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_debug_unit32/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_debug_unit32/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_debug_unit64/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_debug_unit64/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_release_unit32/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_release_unit32/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_release_unit64/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_release_unit64/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_determinism/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_determinism/)|[![Build Status](https://ci.dot.net/buildStatus/icon?job=dotnet_roslyn/dev15.6.x/windows_debug_vs-integration)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_debug_vs-integration/)|[![Build Status](https://ci.dot.net/buildStatus/icon?job=dotnet_roslyn/dev15.6.x/windows_release_vs-integration)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.6.x/job/windows_release_vs-integration/)|
|**dev15.3.x**|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_debug_unit32/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_debug_unit32/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_debug_unit64/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_debug_unit64/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_release_unit32/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_release_unit32/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_release_unit64/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_release_unit64/)|[![Build Status](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_determinism/badge/icon)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_determinism/)|[![Build Status](https://ci.dot.net/buildStatus/icon?job=dotnet_roslyn/dev15.3.x/windows_debug_vs-integration)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_debug_vs-integration/)|[![Build Status](https://ci.dot.net/buildStatus/icon?job=dotnet_roslyn/dev15.3.x/windows_release_vs-integration)](https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_release_vs-integration/)|
Copy link
Member

Choose a reason for hiding this comment

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

This looks very suspicious. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably resolved the merge conflict wrong, let me revert it

@JieCarolHu JieCarolHu force-pushed the features/quick-info-party branch from ba0245d to f04559a Compare April 20, 2018 22:39
@JieCarolHu
Copy link
Contributor Author

@jasonmalinowski Could you please approve this change? Thanks

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

I've reviewed the overall payload and this looks like the right set of files/diffs. I didn't review each file of course since that's already happened.

Before we merge this let's make sure that:

  1. Integration tests are passing.
  2. @KirillOsenkov has confirmed this is a payload they can take for VS for Mac.

Also, please make sure we merge this with a regular merge a not a squash merge. Wouldn't want to lose all the history here.

@jasonmalinowski
Copy link
Member

Also FYI to @Pilchie I think since we'll kinda want ask mode approval on this.

@KirillOsenkov
Copy link
Member

I didn't review the changes but I'm guessing this is fine for VSMac. We are planning to adopt the new Quick Info API in 15.8.

@@ -489,7 +489,6 @@ private static Document GetDocumentToVerify(DocumentId expectedChangedDocumentId
int index, ImmutableArray<CodeAction> actions, CodeActionPriority? priority = null)
{
Assert.NotNull(actions);
Assert.NotEmpty(actions);
Copy link
Member

Choose a reason for hiding this comment

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

What is this change and how is it related to Quick Info work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted. the change was accidentally got in when I was switching branches...

@jinujoseph
Copy link
Contributor

Approved to merge for 15.8.preview2

@JieCarolHu JieCarolHu merged commit be50e10 into master-vs-deps Apr 24, 2018
@DustinCampbell
Copy link
Member

w00t!!!

@jasonmalinowski jasonmalinowski deleted the features/quick-info-party branch May 18, 2018 20:55
@jasonmalinowski jasonmalinowski restored the features/quick-info-party branch May 18, 2018 20:55
@JieCarolHu JieCarolHu deleted the features/quick-info-party branch May 24, 2018 19:06
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.

7 participants