-
Notifications
You must be signed in to change notification settings - Fork 11
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: tweaking relative path handling for windows compatibility #18
Conversation
@jasonpaulos would appreciate if you could trigger a new deployment to npm once this is reviewed and merged |
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.
I can help figure out the best way to support windows paths in the debug adapter, but it will be much easier if we have reliable testing on windows. Since this is an area you've shown interest in, would you be willing to modify the github actions workflow to also build and test on windows?
Thanks @jasonpaulos. I pushed a tweak to ci to include windows worker, will check the remaining comments tomorrow |
@jasonpaulos i'm debugging the windows tests, majority of them fail given the tests using unix like path syntax and i assume they probably weren't tested on windows prior. Will let you know if ill hit blockers that might need your aid. I have a separate pipeline on forked repo where im testing it, you can assume the error on the current pr is already fixed (adding gitattributes for windows to us LS) |
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.
Ok sounds good. I'll just leave these two comments with some insights from my end
@jasonpaulos i pushed interim tweaks for eol. You will now observe the test failures, if some seem obvious to fix please let me know. I assumed its trivial tweak in path strings but seems like it requires digging a bit more into adapter implementation. Also seems like pipeline requires approval upon each trigger - any chance you can disable this for this pr or add myself and @neilcampbell to contributors on the repo? |
You should be able to run the workflow without my approval now |
Thanks, I see the test failures now. I'll see if I can diagnose the problem |
* chore: wip testing windows compatibility * chore: testing
I hope you don't mind but I pushed some changes to this PR. I liked the idea of expanding Specifically, if you have access to a windows machine, can you please run the extension and ensure the |
@jasonpaulos awesome, thanks for pushing the tweaks. Smoke tested this on windows independently and as a dependency in our extension repo - works fine 👍 I can't self approve the repo so feel free to merge as time allows. |
Proposed changes
path
. @jasonpaulos given that its a vscode extension related core dependency - safe to assume we can target node? I think all electron apps got node available