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

Adding link detector regex to support c# extension #30882

Closed

Conversation

rajkumar42
Copy link
Contributor

Adding link detector regex for file links of the following format

at app1.Program.Foo() in c:\temp\dotnet\app1\Program.cs:line 9

at app1.Program.Foo() in c:\temp\dotnet\app1\Program.cs:line 9
@mention-bot
Copy link

@rajkumar42, thanks for your PR! By analyzing the history of the files in this pull request, we identified @michelkaporin and @bpasero to be potential reviewers.

@rajkumar42
Copy link
Contributor Author

Please review @gregg-miskelly @isidorn

@gregg-miskelly
Copy link
Member

Since our matching at the end of the string isn't very strong, I think we might want to tighten up the start of path matching to only support Drive-colon ([a-zA-Z]:\) or Windows network share syntax (\\) and Unix path syntax (/). Ex: Foo/Program.cs:line 9 shouldn't match

@gregg-miskelly
Copy link
Member

Using \w to match the characters in line isn't ideal as -- \w includes numbers, which we definitely don't want, and it sounds like it doesn't include accented characters. Can you try \p{L}?

@gregg-miskelly
Copy link
Member

Two other thoughts:

  1. Seems like the line number should be required (no ?) at the end
  2. Seems like we should require it to be the line ending ($ at the end)

@michelkaporin
Copy link
Contributor

Agree with @gregg-miskelly comments.

Is it possible to also address #28508 for c# regex while you are on it? This regex suffers of the same problem.

Besides that it looks good to me.

"(?:[\\/][^<>\\/\'\"\[\]:]*)+"+ // File path
")" +
":line\ " + // ends with :line literal
"(\d+)", // Line number
Copy link
Member

@gregg-miskelly gregg-miskelly Jul 19, 2017

Choose a reason for hiding this comment

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

I might suggest adding a '$' also for end-of-line after \d+

Copy link
Member

@gregg-miskelly gregg-miskelly left a comment

Choose a reason for hiding this comment

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

Assuming that the first RegEx only needs to match full paths, LGTM.

@michelkaporin
Copy link
Contributor

My VS Code instance freezes with the latest changes.

Here is the code I am testing it with, if it helps:

console.log('http://test.com:8080');
console.log('Relative paths:');
console.log('.vscode\\launch.json:5:11'); // relative path
console.log('.vscode/launch.json:5:11'); // relative path
console.log('./linkDetection.ts:5:11');
console.log('linkDetection.ts:5:11');
console.log('linkDetection.ts:15');
console.log('test (src/command/AsyncCommand.ts:6:24)');
console.log('Full paths:');
console.log('c:/Users/t-mikapo/Documents/Projects/multiline-object-properties/out/linkDetection.js:5:11');
console.log('c:\\Users\\t-mikapo\\Documents\\Projects\\multiline-object-properties\\out\\linkDetection.js:5:11');
console.log('c:/Users/t-mikapo/Documents/Projects/multiline-object-properties/out/linkDetection.js:5');
console.log('c:\\Users\\t-mikapo\\Documents\\Projects\\multiline-object-properties\\out\\linkDetection.js:5');
console.log('Other:');
console.log('c:/Users/t-mikapo/Documents/Projects/multiline-object-properties/out/linkDetection.js:5:11 combined with ./linkDetection.ts:5)');
console.log('at model.newPostInstance (.vscode/launch.json:11:3)');
console.log('at model.newPostInstance (.vscode/launch.json on line 8, column 13)');
console.log('at model.newPostInstance (.vscode/launch.json:(45,18))');
console.log('d:\\proj\\clicon\\Program.cs:line 23');
console.log('d:\\proj\\clicon\\Program.cs:line 23, column 13');
console.log('d:\\proj\\clicon\\Program.cs line 23, column 13');
console.log('file:\\\c:\\Users\\t-mikapo\\Documents\\Projects\\multiline-object-properties\\out\\linkDetection.js:5:11');

@michelkaporin
Copy link
Contributor

@rajkumar42 with your latest commit it does not match C# syntax any more. Please verify your changes before commiting, and let me know once ready for a final review 👍

@rajkumar42
Copy link
Contributor Author

@michelkaporin I couldn't reproduce the issue, my testing is limited to mac. I've pushed a change from single line to multiline. Can you please check if that fixes the issue?

@michelkaporin
Copy link
Contributor

@isidorn would you be able to do a final review of that as I am out please? I was using the code above to test this PR on Windows.

@gregg-miskelly
Copy link
Member

Ping

@isidorn
Copy link
Contributor

isidorn commented Aug 28, 2017

Sorry for the slow response, Michel finished his internship and I was on vacation,
I will have to re-review this and that I can only do after endgame -> September

@isidorn isidorn added this to the September 2017 milestone Aug 28, 2017
@isidorn
Copy link
Contributor

isidorn commented Sep 8, 2017

Thanks for your PR and sorry that I am only now looking into this.
This PR is very C# specific, a better approach to solving this would be to allow extensions to somehow contribute different link detection mechanisms.
However currently all our link detection is not in the best state and rather than accepting PRs that complicate the situation further I would prefer that we somehow restrucutre and improve our Link detection. For that I have created a debt item #34026

Due to the reasons above I will not accept this PR.

A PR which I would be willing to accept is one that is language agnostic and has a less cryptic structure - not a complex regex which seems very hard to maintain and modify imho

@isidorn isidorn closed this Sep 8, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
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.

6 participants