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

Improve LinkDetector #81336

Merged
merged 1 commit into from
Sep 25, 2019
Merged

Improve LinkDetector #81336

merged 1 commit into from
Sep 25, 2019

Conversation

dgozman
Copy link
Contributor

@dgozman dgozman commented Sep 23, 2019

@isidorn Could you please take a look? This implements some improvements. We can try to unify with terminal link handling, however it doesn't seem to be straightforward because the control flow is very different.

  • recognizes web urls;
  • checks that file path exists;
  • supports relative to workspace root paths (./foo);
  • supports relative to home dir paths (~/foo);
  • optional line/column;
  • smoke test.

@kieferrm kieferrm requested a review from isidorn September 24, 2019 01:57
@isidorn isidorn added this to the September 2019 milestone Sep 24, 2019
@isidorn isidorn added the debug Debug viewlet, configurations, breakpoints, adapter issues label Sep 24, 2019
@isidorn
Copy link
Contributor

isidorn commented Sep 24, 2019

I really really like this work. I just left one minor comment in the code.
It's great that you added tests!

I did not code review the linkDetector.ts. I trust that you did it properly and If the tests are good and if it is behaving fine it is perfetly fine for me - that is why I would like it to be pushed to insiders so we start self hosting on it. Also I hope that it is ok for you if I from now on treat you as the owner of the linkDetector, thus I might ping you on potential incoming issues?
Also if there is some part of the linkDetector that you would like me to review let me know and I can give it a deeper review. Thanks!

@dgozman dgozman force-pushed the link-detector branch 2 times, most recently from 959677b to 3d44ddd Compare September 24, 2019 18:20
- recognizes web urls;
- checks that file path exists;
- supports relative to workspace root paths (./foo);
- supports relative to home dir paths (~/foo);
- optional line/column;
- smoke test.
Copy link
Contributor Author

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Thank you, this sounds good to me. I did address the comments.

There was a small change in behavior - we don't wrap text with extra span when not required. From manual testing, I didn't find any issues with that.

src/vs/workbench/contrib/debug/common/replModel.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/common/replModel.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/linkDetector.ts Outdated Show resolved Hide resolved
@isidorn
Copy link
Contributor

isidorn commented Sep 25, 2019

Great, thanks for addressing the comments. Merging this in so we start self hosting on it already.
Thanks again for this contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants