Skip to content

feat: handle windows specific paths in log-path parameter #512

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

Merged
merged 1 commit into from
Jun 23, 2025

Conversation

ibetitsmike
Copy link
Contributor

@ibetitsmike ibetitsmike commented May 22, 2025

Fixes: #430

@ibetitsmike ibetitsmike force-pushed the mike/430-vs-code-win-path branch from 44e3dfa to ab2f929 Compare June 10, 2025 16:28
@bcpeinhardt
Copy link
Collaborator

bcpeinhardt commented Jun 10, 2025

I suspect the issue was that claude nuked your lockfile (it likes to do that). Just reverting the lockfile fixed the build. I'll leave passing the test and lint checks up to you :)

@ibetitsmike ibetitsmike force-pushed the mike/430-vs-code-win-path branch from bdc35ba to 281fe1a Compare June 17, 2025 17:28
@ibetitsmike ibetitsmike force-pushed the mike/430-vs-code-win-path branch from 281fe1a to bd9d1ca Compare June 17, 2025 17:47
@ibetitsmike ibetitsmike reopened this Jun 17, 2025
@matifali matifali requested a review from jaggederest June 18, 2025 07:24
@jaggederest jaggederest requested a review from code-asher June 18, 2025 17:28
Copy link
Contributor

@jaggederest jaggederest left a comment

Choose a reason for hiding this comment

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

This seems fine to me, but I am not yet competent to hand out the ✅ of approval so I've asked Asher to take a look 👍

@code-asher
Copy link
Member

code-asher commented Jun 20, 2025

It seems like a reasonable change to me since we already use escapeCommandArg() on other path arguments. It would be nice to see a test for updateSSHConfig().

The issue says we are printing --log-dir C%3A%5CUsers%5Cmicha%5Clogs, so I assume now we will be printing --log-dir "C%3A%5CUsers%5Cmicha%5Clogs" which might work (or at least not error), but it seems like we should really be printing --log-dir "C:\Users\micha\logs".

Do we know where the escaped chars came from?

@code-asher
Copy link
Member

Oh duh it was using escape before. Looks good to me. I do think we should test this stuff eventually.

@code-asher
Copy link
Member

code-asher commented Jun 20, 2025

I know what happened, escapeCommandArg used to be called escape and was local to updateSSHConfig, then formatLogArg was broken out into its own function, with no access to the local escape, but escape happens to be a built-in function so no errors were thrown lol

@ibetitsmike
Copy link
Contributor Author

Oh duh it was using escape before. Looks good to me. I do think we should test this stuff eventually.

@code-asher as in 'unit test'?

@code-asher
Copy link
Member

Unit test would be ideal yeah! But I think updateSSHConfig() would need to be broken out of Remote. Might not be worth the effort right now, feel free to merge.

@mtojek mtojek merged commit d1289c6 into main Jun 23, 2025
4 checks passed
@mtojek mtojek deleted the mike/430-vs-code-win-path branch June 23, 2025 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proxy log directory doesn't handle Windows style \
6 participants