Checks integrated terminal output for more types of relative paths #22602
Conversation
|
@markwpearce, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Tyriar to be a potential reviewer. |
|
@markwpearce, It will cover your contributions to all Microsoft-managed open source projects. |
|
@markwpearce, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
There was a problem hiding this comment.
Thanks @markwpearce, this is great. I made some initial comments but I haven't played around with it yet.
| @@ -41,6 +65,7 @@ suite('Workbench - TerminalLinkHandler', () => { | |||
| testLink('c:/a/long/path'); | |||
| testLink('c:\\a\\long\\path'); | |||
| testLink('c:\\mixed/slash\\path'); | |||
| testLink('a/relative/path'); | |||
There was a problem hiding this comment.
Will this also linkify just plain words for example "foo"? That way after validation the output of ls should all be linked. If so a test for that would be great.
There was a problem hiding this comment.
Yes, it should. I'll add a test.
There was a problem hiding this comment.
Actually now that I think about it that's probably not a good idea until we know more about the current working directory within the terminal. It could get confusing otherwise. Right now wealways open files relative to VS Code's folder, not the terminal's.
| } | ||
|
|
||
| private _resolvePath(link: string): TPromise<string> { | ||
| link = this._preprocessPath(link); |
There was a problem hiding this comment.
Was this factoring out purely for code cleanliness?
There was a problem hiding this comment.
Mostly to to test link preprocessing (_preprocessPath() method) in isolation, and not have to worry about it returning a promise in the tests.
|
I've noticed three issues in this PR:
Given that 3 is a bigger problem than 2, I'll solve issue 1 for now, and to be links, outputted file paths will just need to have a path separator in them. |
|
@markwpearce on point 3, this is actually a bug in xterm.js thanks for finding it. The intent was to linkify multiple links on the same row using a single link matcher, created xtermjs/xterm.js#612 |
There was a problem hiding this comment.
Really high quality PR @markwpearce thanks! I created a follow up to do it for paths with no separator.
| } else { | ||
| // Resolve workspace path . / .. -> <path>/. / <path/.. | ||
| if (link.charAt(0) === '.') { | ||
| if (!this._contextService.hasWorkspace) { |
Integrated terminal output is now checked for relative paths of the format
src/app/file.ts, that is, relative paths without a leading./These types of relative paths are checked to make sure the file they are referring to actually exists.
Some tests were added to verify the paths are properly formatted in order to check for the file.
Note: This was not tested in a Windows environment
Fixes #22528