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

Move breakpoint resolver down into Features #28321

Merged
merged 5 commits into from
Dec 14, 2019

Conversation

mhutch
Copy link
Member

@mhutch mhutch commented Jul 6, 2018

Moves the IBreakpointResolutionService implementations from the VisualStudio layer down into the EditorFeatures layer. They don't have VS dependencies, and the interface is already defined in this layer. This gives VSMac the option to use this service without code duplication.

Brings IProximityExpressionsService and its implementations along because it doesn't really have any reason to be in the higher layer either and could also potentially be useful.

@mhutch mhutch requested a review from a team as a code owner July 6, 2018 00:46
@tmat
Copy link
Member

tmat commented Jul 6, 2018

Do these types have dependencies on the Editor? Just wondering if we can move them even lower, to the Features layer.

@mhutch
Copy link
Member Author

mhutch commented Jul 6, 2018

I think that would be possible but I'm not sure whether it matters 🙂

@CyrusNajmabadi
Copy link
Member

@mhutch Generally, this is useful because it really helps understand the layering. It also prevents people from unintentionally taking on dependencies later that aren't appropriate (at least without some discussion).

PLacing this in features would mean this couldn't accidently take an editor dependency without people strongly considering if that was an ok idea or not.

@jinujoseph jinujoseph added this to the 16.0 milestone Jul 6, 2018
@jasonmalinowski
Copy link
Member

Yeah, we should move this to Features at least, since we really don't want this accidentally taking an editor dependency.

@mhutch mhutch force-pushed the debugger-resolver-in-editor-features branch from 3e04c23 to cc933f8 Compare July 10, 2018 15:36
@mhutch mhutch changed the title Move breakpoint resolver impls down into EditorFeatures Move breakpoint resolver down into Features Jul 10, 2018
@mhutch
Copy link
Member Author

mhutch commented Jul 10, 2018

Okay, moved it!

@mhutch mhutch force-pushed the debugger-resolver-in-editor-features branch from cc933f8 to 9b43615 Compare July 10, 2018 22:55
@mhutch mhutch force-pushed the debugger-resolver-in-editor-features branch from 9b43615 to b534032 Compare September 6, 2018 17:33
@mhutch
Copy link
Member Author

mhutch commented Sep 11, 2018

Fixed conflicts. Failures do not appear to be related. @jasonmalinowski what do i need to do to move this along?

@jasonmalinowski
Copy link
Member

@jinujoseph can we get @mhutch a PR buddy for this one?

@jinujoseph
Copy link
Contributor

@tmat can you help buddy this forward

@mhutch
Copy link
Member Author

mhutch commented Oct 5, 2018

bump?

@mhutch
Copy link
Member Author

mhutch commented Nov 8, 2018

bump

@tmat
Copy link
Member

tmat commented Nov 27, 2018

Looks good modulo a couple of nits. We need to be careful with merging though as this will break F# and possibly TypeScript. We need to coordinate.

@sharwell @jasonmalinowski @TIHan @amcasey

How about

  1. creating a branch off of master-vs-deps with this change, produce a signed build
  2. create VS insertion PR from that branch
  3. create corresponding change in TS and F# and insert them in the same PR [2]

We can avoid the branch if we are willing to block our insertions while waiting on TS and F# changes.

@amcasey
Copy link
Member

amcasey commented Nov 27, 2018

FYI @minestarks

@tmat
Copy link
Member

tmat commented Nov 27, 2018

test this please

@minestarks
Copy link

minestarks commented Nov 27, 2018

@tmat speaking for TypeScript: please, pretty please can you consider doing this breaking change in two stages, where you duplicate the API, mark the old one as obsolete, and THEN after we've moved over, remove the old API? Coordinated insertions are have been a routine headache for us. Let's discuss over e-mail if you wish.

@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Nov 28, 2018
@mhutch
Copy link
Member Author

mhutch commented Jan 8, 2019

Bump :)

@tmat
Copy link
Member

tmat commented Jan 8, 2019

@mhutch Still on my radar. We are working on a system that will allow us to make changes like this that affect multiple components across VS easier. Once we have that system in place we will merge this change.

@jinujoseph jinujoseph modified the milestones: 16.0, 16.1 Jan 17, 2019
@jinujoseph jinujoseph modified the milestones: 16.1, 16.3 Jun 9, 2019
@jinujoseph jinujoseph modified the milestones: 16.3, Backlog Sep 9, 2019
@tmat tmat force-pushed the debugger-resolver-in-editor-features branch from b534032 to ab004cf Compare December 10, 2019 21:25
@JoeRobich JoeRobich requested review from a team as code owners December 10, 2019 21:25
@tmat
Copy link
Member

tmat commented Dec 10, 2019

@mhutch The code base is now ready for this change. I have resolved conflicts.

@tmat
Copy link
Member

tmat commented Dec 12, 2019

@dotnet/roslyn-ide Anybody wants to take a look?

@tmat tmat force-pushed the debugger-resolver-in-editor-features branch from a22e5d5 to 8098cfe Compare December 12, 2019 22:20
@tmat tmat merged commit e8a539c into dotnet:master Dec 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants