-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should. I'll add a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this factoring out purely for code cleanliness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
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