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.

Sign up for GitHub

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

Jump to bottom

Code wrapping refactorings should be lower pri than other refactorings #33631

Merged
merged 5 commits into from
Feb 26, 2019
Merged

Code wrapping refactorings should be lower pri than other refactorings #33631

merged 5 commits into from
Feb 26, 2019

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Feb 23, 2019 •
edited
Loading

Fixes #33605

After this change, lightbulb list looks like:

image

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner February 23, 2019 22:25
@CyrusNajmabadi
Copy link
Member Author

tagging @jcouv @sharwell @DustinCampbell @dotnet/roslyn-ide

@jinujoseph
Copy link
Contributor

@mavasani did not we resolve this via #32435 ?

@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Feb 25, 2019
@CyrusNajmabadi
Copy link
Member Author

@jinujoseph It was resolved solely for those two refactorings. My change resolves it for all refactorings.

@jinujoseph jinujoseph added this to the 16.1.P1 milestone Feb 26, 2019
@mavasani mavasani merged commit 8d53065 into dotnet:master Feb 26, 2019
@CyrusNajmabadi
Copy link
Member Author

Thanks!

@CyrusNajmabadi CyrusNajmabadi deleted the wrapPriority branch February 26, 2019 21:17
@mavasani mavasani mentioned this pull request Mar 1, 2019
Merged
mavasani added a commit to mavasani/roslyn that referenced this pull request Mar 1, 2019
@mavasani
Merge pull request dotnet#33631 from CyrusNajmabadi/wrapPriority
c9183bd 
Code wrapping refactorings should be lower pri than other refactorings

(cherry picked from commit 8d53065)
mavasani added a commit that referenced this pull request Mar 2, 2019
@mavasani
Merge pull request #33790 from mavasani/WrappingPort
4de954e 
Port #33631 to dev16.0 (Low priority for code wrapping refactorings)
@heejaechang
Copy link
Contributor

I think we should discuss how to solve ordering problem for refactoring.

I believe this kind of heuristic will require an endless tweak as new refactoring are added.

I opened an issue - #33818

@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Mar 2, 2019
Closed
@CyrusNajmabadi
Copy link
Member Author

I believe this kind of heuristic will require an endless tweak as new refactoring are added.

NOte: this was not a tweak. This was literally just a bug. The problem here was that we have a system for prioritizing refactorings. However, the implementation was buggy, and a low-pri refactoring wasn't showing up after medium pri refactorings if that refactoring had nested refactorings.

Nothing about the actual prioritization design changed here. We just fixed up the impl to match teh intended design. So i question if anything needs to be done here.

333fred added a commit to 333fred/roslyn that referenced this pull request Mar 4, 2019
@333fred
Merge remote-tracking branch 'dotnet/master' into HEAD
f81bf24 
* dotnet/master: (903 commits)
  Add UseEnhancedColors to FeatureOnOffOptionsProvider (dotnet#33802)
  Update TypeStyle.cs
  Text on a InterpolatedVerbatimStringStartToken token (dotnet#33747)
  added ability to clear all diagnostics reproted from a IDiagnosticUpd… (dotnet#33805)
  Use a set instead of map to define processed redundant nodes.
  EnC: Remove dependency on solution from AnalyzeDocumentAsync (dotnet#33796)
  Add workitem links to new redundant assignment tests.
  Move leading trivia with node when removing unused values.
  Remove OOP related feature options. (dotnet#31644)
  Merge pull request dotnet#33631 from CyrusNajmabadi/wrapPriority
  Use the correct iteration count in IterationDataAttribute
  Handle interface members in NullableWalker.AsMemberOfType (dotnet#33764)
  Cleanly work around microsoft/nodejstools#2138
  Implement full spec changes for Index/Range (dotnet#33679)
  Further reduce expectations on deep fluent calls
  Lower expectation for deep fluent call
  Take screenshot after writing logs
  Avoid throwing exception in static constructor
  Offer to add parameter to constructor with no existing parameters (dotnet#33624)
  Add regression test for nullable crash (dotnet#33735)
  ...
@sharwell sharwell mentioned this pull request Mar 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mavasani mavasani mavasani approved these changes

Assignees

@mavasani mavasani

Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
16.1.P1
Development

Successfully merging this pull request may close these issues.
4 participants
@CyrusNajmabadi @jinujoseph @heejaechang @mavasani