Skip to content

Conversation

@damuellen
Copy link
Contributor

getenv generates a warning on windows.

getenv generates a warning on windows.
@natecook1000
Copy link
Member

Thanks, @damuellen! Are you able to test this with one of the supported shells under Windows? I know we don't have automated tests for this, but it would be good to know the status.

@natecook1000
Copy link
Member

cc @compnerd

@compnerd
Copy link
Collaborator

Honestly, I'm not sure if it's too important to test (beyond compiling). The windows environment doesn't have a SHELL environment variable, your shell is either cmd or powershell.

var length: Int = 0
guard _dupenv_s(&buffer, &length, "SHELL") != 0, let shellVar = buffer else { return nil }
let shellParts = String(cString: shellVar).split(separator: "/")
free(buffer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would swap the two things, doing a defer { free(buffer) } and then just returning the processed value. However, the separator is unlikely to be / and more likely to be \ if there was such a variable, though it could be either.

@natecook1000
Copy link
Member

natecook1000 commented Oct 24, 2020

The windows environment doesn't have a SHELL environment variable, your shell is either cmd or powershell.

🤔 In that case, given that bash, fish, and zsh are the only three choices, should CompletionShell.autodetected() just return nil here?

@compnerd
Copy link
Collaborator

Yeah, returning nil for the time being sounds reasonable. For powershell, it may be possible to auto complete, but I don't know about cmd.

@damuellen
Copy link
Contributor Author

I will close this pr, and create a new own which simply return nil on windows.

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.

3 participants