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

Added support for links of file paths with spaces (#13245) #43733

Merged
merged 3 commits into from Mar 7, 2018

Conversation

Projects
None yet
5 participants
@DominikDitoIvosevic
Copy link

DominikDitoIvosevic commented Feb 15, 2018

Hi.
This is just a regex improvement to capture links with spaces as well.
The approach is to use an OR non capturing group which does (validChar | space+validChar)* instead of previously using just validChar*

Fixes:
#13245
and
Microsoft/AL#167

@DominikDitoIvosevic

This comment has been minimized.

Copy link

DominikDitoIvosevic commented Feb 15, 2018

I see that I have 1 failing:

  1. workspace-namespace findFiles:
    AssertionError: 0 == 1

    • expected - actual
      -0
      +1

    at C:\projects\vscode\extensions\vscode-api-tests\src\singlefolder-tests\workspace.test.ts:499:11

Is this a know instability or is it using this link pattern matching?
For some reason I cannot debug or run this test. When I do

PS C:\git\vscode> .\scripts\test.bat --run "C:/git/vscode/extensions/vscode-api-tests/out/singlefolder-tests/workspace.test.js"

it hangs and doesn't do anything.

Also, I see that 1 of the 3 travis jobs failed after 50 mins for hanging.
Could these issues be related to my changes?

@bpasero bpasero assigned isidorn and unassigned bpasero Feb 15, 2018

@isidorn isidorn added this to the February 2018 milestone Feb 16, 2018

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Feb 16, 2018

@DominikDitoIvosevic thanks for your PR.
Great that you have written tests. On top of that you could update the comment above the regex since it is quite complex so to make it easier for understanding.

Travis failing you can ignore. However unit tests you probably broke since they are passing nicely wihtout your changes.
Please look into fixing them.
You can run the unit tests by using the launch configuration "Unit Tests"
Here's our contributing document which should have more info on this subject https://github.com/Microsoft/vscode/wiki/How-to-Contribute

Merge branch 'master' of https://github.com/DominikDitoIvosevic/vscode
…into bugs/13245/doivosev/linkSpaceCaptureFix
@DominikDitoIvosevic

This comment has been minimized.

Copy link

DominikDitoIvosevic commented Feb 22, 2018

@isidorn I was locally having issues with running tests because I was using the default git checkout style using clrf and your tests are hardcoded to work with LF. I see that after remerging with master my tests pass on appveyor but travis still fails. Does this mean the last fail was an instability and my PR is ok?

@isidorn isidorn modified the milestones: February 2018, March 2018 Feb 22, 2018

@DominikDitoIvosevic

This comment has been minimized.

Copy link

DominikDitoIvosevic commented Feb 22, 2018

I tried allowing spaces in all the file patterns but there seems to be a bigger issue here. You have to choose between more path characters or more variations from which we can extract paths.

One problem is that while on Windows <>" etc are invalid path characters they are valid on linux. Also, one test wasn't catching ] and ' which are valid on all operating systems so I suppose that was a mistake. The reason why the last of the 3 patterns doesn't allow spaces in the path is because you allow for not specifying neither the line nor the character for that pattern and since space is a valid character in the file extension it's not possible to match your current tests.

While I think it's important to support spaces for all path patterns, allowing spaces for the last of the 3 patterns would require you to enforce specifying some character after the path. For instance some tests have in after the path but none of the patterns require the in to be there. We could split the last pattern to allow for neither the line or the char but to require in after the path. That decision is up to you.

@DominikDitoIvosevic

This comment has been minimized.

Copy link

DominikDitoIvosevic commented Feb 26, 2018

@isidorn could you please tell me if there is something else I need to do? Could we get a review started up?
I've split the patterns into template literals to hopefully make it more clear what's going on.

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Feb 26, 2018

@DominikDitoIvosevic Hi, this week we are in the endgame week, which means we only take limited code changes we are cruical for the release.
Due to that I will review this PR next week in our debt week. Than I will provide feedback. Thanks

@DominikDitoIvosevic

This comment has been minimized.

Copy link

DominikDitoIvosevic commented Mar 5, 2018

@isidorn Does this mean this fix is potentially getting into 1.22 or 1.23?

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Mar 5, 2018

@DominikDitoIvosevic 1.22

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Mar 6, 2018

@DominikDitoIvosevic I checked out your changes and they make sense to me. I would merge them in so we try them out in insiders and see if something breaks.
However also adding @alexandrudima since he is the original author looking at commits to see if he objects about some changes here.

@DominikDitoIvosevic did you bild vscode with your changes and verify nothing gets badly broken? Like detecting links in various outputs?

@alexandrudima

This comment has been minimized.

Copy link
Member

alexandrudima commented Mar 7, 2018

@isidorn Adding @bpasero. He is the original author of the output link computer.

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Mar 7, 2018

Let's merge this in and see how it behaves in the wild.

@isidorn isidorn merged commit d4c5873 into Microsoft:master Mar 7, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment