Skip to content
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

Run fish activation scripts with source instead of . #7343

Closed
brettcannon opened this issue Sep 12, 2019 · 9 comments
Closed

Run fish activation scripts with source instead of . #7343

brettcannon opened this issue Sep 12, 2019 · 9 comments
Assignees
Labels
area-terminal feature-request Request for new features or functionality good first issue

Comments

@brettcannon
Copy link
Member

Welcome to fish, the friendly interactive shell
 (master *%) > source /Users/brettcannon/Repositories/python/voters/.venv/bin/activate.fish

That should have been:

. /Users/brettcannon/Repositories/python/voters/.venv/bin/activate.fish

(Note the . instead of source.)

@brettcannon brettcannon added bug Issue identified by VS Code Team member as probable bug triage-needed Needs assignment to the proper sub-team labels Sep 12, 2019
@kimadeline kimadeline self-assigned this Sep 12, 2019
@ghost ghost removed the triage-needed Needs assignment to the proper sub-team label Sep 12, 2019
@kimadeline

This comment has been minimized.

@kimadeline
Copy link

getActivationCommandsForInterpreter() always calls source <activation file>:

public async getActivationCommandsForInterpreter(pythonPath: string, targetShell: TerminalShellType): Promise<string[] | undefined> {
const scriptFile = await this.findScriptFile(pythonPath, this.getScriptsInOrderOfPreference(targetShell));
if (!scriptFile) {
return;
}
return [`source ${scriptFile.fileToCommandArgument()}`];
}

@faho
Copy link

faho commented Sep 18, 2019

That should have been:

. /Users/brettcannon/Repositories/python/voters/.venv/bin/activate.fish

(Note the . instead of source.)

That can't be what's going on. In not-ancient fish, . is merely a compatibility function with source being a proper builtin. At best they do the exact same thing.

@brettcannon
Copy link
Member Author

@faho huh, something funky happened on my shell instance that day then since . worked but source didn't.

@ghost ghost removed the needs PR label Sep 18, 2019
@faho
Copy link

faho commented Sep 19, 2019

I recommend reverting #7387. It's useless complexity.

@brettcannon
Copy link
Member Author

@faho any interest in submitting a PR to revert it?

@faho
Copy link

faho commented Sep 19, 2019

To be honest, no.

I don't use vscode and I've never had a python app where I had to use venv (mostly really tiny scripts and such), so I couldn't really test it. I could do the git bits, but that's just git revert 40a1d045f925bb1c3e7d7424e9d010297253cc78.

@brettcannon brettcannon reopened this Sep 19, 2019
@ghost ghost added the triage-needed Needs assignment to the proper sub-team label Sep 19, 2019
@ghost ghost removed the triage-needed Needs assignment to the proper sub-team label Sep 19, 2019
@brettcannon brettcannon added triage-needed Needs assignment to the proper sub-team good first issue feature-request Request for new features or functionality and removed reason-preexisting bug Issue identified by VS Code Team member as probable bug labels Sep 19, 2019
@brettcannon brettcannon changed the title Incorrectly sourcing fish venv activation script Run fish activation scripts with source instead of . Sep 19, 2019
@brettcannon brettcannon removed the triage-needed Needs assignment to the proper sub-team label Sep 19, 2019
@DonJayamanne
Copy link

I introduced the change, hence fitting i revert it 😄

@karrtikr
Copy link

validated

@ghost ghost removed the needs PR label Sep 23, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-terminal feature-request Request for new features or functionality good first issue
Projects
None yet
Development

No branches or pull requests

7 participants