Skip to content

Implements run to line. #130039

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

Merged
merged 4 commits into from
Aug 3, 2021
Merged

Implements run to line. #130039

merged 4 commits into from
Aug 3, 2021

Conversation

hediet
Copy link
Member

@hediet hediet commented Aug 3, 2021

This PR fixes #123872

I'm unsure if I got column 0/undefined right.

recording

What do you think of putting "Run to line" to the top of that menu? 😉

@hediet hediet requested a review from isidorn August 3, 2021 13:23
@hediet hediet force-pushed the hediet/run-to-line branch from 024cc8a to 5edf7a1 Compare August 3, 2021 13:29
@hediet hediet self-assigned this Aug 3, 2021
@hediet hediet added this to the August 2021 milestone Aug 3, 2021
@isidorn
Copy link
Contributor

isidorn commented Aug 3, 2021

Did a first quick review and it looks good.

  1. Seems like you have unused imports in debugEditorActions. Can you please fix that
  2. "Run to Line" should be at the bottom of the menu so we do not break users muscle memory. Probably a lot of users memorised that adding Conditional Breakpoints is the second option, and putting a new command on the top would shift everything

I will do one more detailed review soon.

@isidorn
Copy link
Contributor

isidorn commented Aug 3, 2021

Can you please implement the newly introduced method in the mockDebugService in src/vs/workbench/contrib/debug/test/browser/mockDebug.ts

@hediet
Copy link
Member Author

hediet commented Aug 3, 2021

I really should check the logs before pushing :D

@isidorn
Copy link
Contributor

isidorn commented Aug 3, 2021

Yeah that can be helpful :D

@isidorn
Copy link
Contributor

isidorn commented Aug 3, 2021

Run to line should only be there while debugging is active

Screenshot 2021-08-03 at 17 10 53

Apart from this all seems good. Once you tackle this I will merge. Thanks a lot 👏

@hediet
Copy link
Member Author

hediet commented Aug 3, 2021

I agree. Wouldn't it be nice though if "Run to line" starts debugging if debugging is not active?

@isidorn
Copy link
Contributor

isidorn commented Aug 3, 2021

Yeah I also thought about that. The problem is that starting debugging we can not guarantee that the current file will actually get run.
Though I like the idea in general and we can go down that approach to try it out.

@hediet
Copy link
Member Author

hediet commented Aug 3, 2021

Yeah I also thought about that. The problem is that starting debugging we can not guarantee that the current file will actually get run.
Though I like the idea in general and we can go down that approach to try it out.

I see. I'm okay with hiding it for now!

@isidorn
Copy link
Contributor

isidorn commented Aug 3, 2021

Cool, then let's merge it in. Thanks a lot for this great PR 👏

@isidorn isidorn merged commit 1a0213f into main Aug 3, 2021
@isidorn isidorn deleted the hediet/run-to-line branch August 3, 2021 15:18
@scybert
Copy link

scybert commented Aug 7, 2021

@isidorn I'm curious why you are merging a failing PR.

@isidorn
Copy link
Contributor

isidorn commented Aug 9, 2021

@scybert because I double checked that it is good and that it works as expected. It might have been failing in the past, before latest commits were added.

@hediet
Copy link
Member Author

hediet commented Aug 9, 2021

Also, your tests are flakey...

@github-actions github-actions bot locked and limited conversation to collaborators Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debugger: Put "Run To Cursor" Into Breakpoint Context Menu
3 participants