Skip to content

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

Merged
mtojek merged 1 commit intomainfrom
mike/430-vs-code-win-path
Jun 23, 2025
Merged

feat: handle windows specific paths in log-path parameter#512
mtojek merged 1 commit intomainfrom
mike/430-vs-code-win-path

Conversation

@ibetitsmike
Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator 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
Copy Markdown
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