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

Fix for #200257 plus existing trailing non-numerics regex fix #200919

Merged
merged 10 commits into from
Dec 15, 2023
Merged

Fix for #200257 plus existing trailing non-numerics regex fix #200919

merged 10 commits into from
Dec 15, 2023

Conversation

sparxooo
Copy link
Contributor

This is a draft PR for a fix for issue #200257

Whilst figuring how to fix the issue I created, it appeared that the regex at terminalLinkOpeners.ts:111 was incorrect. Here are the old and new regex side by side, with what I expect the original coder intended. I also tested grep output with filenames/line numbers, and that seems to be correct also.

orig-ruby-regex
fixed-ruby-regex
grep-regex-ok

The trailing period issue has also been fixed with a new text.replace step with (hopefully) a correct regex, that currently only removes trailing periods from line/column numbers (for instance, "test.ts." should be left alone).

The big question possibly is, is there any reason why a trailing period on a link should remain? If not, I guess you could just remove any trailing period regardless of it's preceding text.

@sparxooo
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Love the tests 👏

@Tyriar Tyriar added this to the December / January 2024 milestone Dec 15, 2023
@sparxooo
Copy link
Contributor Author

sparxooo commented Dec 15, 2023

Latest commit I have updated the regex at terminalLinkOpeners.ts:111 in-line with Tyriar's suggestion.

I have updated the TerminalSearchLinkOpener test suite with extra tests for the altered Grep/Ruby regex, plus the trailing period regex. These tests include both line number only, and line / column number tests. I also added line number only tests to the "workspace with spaces" test for completeness.

@Tyriar Tyriar marked this pull request as ready for review December 15, 2023 17:53
Tyriar
Tyriar previously approved these changes Dec 15, 2023
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Great work! 👏

@sparxooo
Copy link
Contributor Author

Thanks Tyriar

Regarding the trailing period issue, although this will catch this:
Check your code at Test.ts:10.

It won't catch this:
Check your code in Test.ts.

If I changed the trailing period regex line to
text = text.replace(/\.$/, '');

Then it would gracefully handle Test.ts., but would in theory break anyone using trailing periods in filenames (if that is even going to be a concern).

…eriod tests for trailing periods without line numbers
@Tyriar
Copy link
Member

Tyriar commented Dec 15, 2023

It's hard to say what the right behavior is here as these links are meant to be very simple and driven by the terminal.integrated.wordSeparators setting. I guess it is preferable to go with the optimistic case and remove the . as excluding it will show results both with and without the ., but including it will only show results including the .

@Tyriar Tyriar merged commit a587752 into microsoft:main Dec 15, 2023
6 checks passed
yiliang114 pushed a commit to yiliang114/vscode that referenced this pull request Jan 3, 2024
Fix for microsoft#200257 plus existing trailing non-numerics regex fix
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2024
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.

4 participants