fix #1147 (search environments.txt file for conda environments) #1240
Conversation
| return getInterpreterDisplayName(pythonPath) | ||
| .catch(() => defaultValue); | ||
| } | ||
| } |
| private locators: IInterpreterLocatorService[] = []; | ||
| constructor(private virtualEnvMgr: VirtualEnvironmentManager) { | ||
| const versionService = new InterpreterVersionService(); | ||
| // The order of the services is important |
There was a problem hiding this comment.
Is not ending comments in a period a thing in the TypeScript world? (Serious question because it's definitely a thing to always use a period in Python.) Otherwise I would definitely use an exclamation point here. 😄 (And if you don't have a preference then I vote for putting in periods to know that a comment definitely is finished and not accidentally incomplete.)
There was a problem hiding this comment.
Is not ending comments in a period a thing in the TypeScript world
Never heard of this.
| } | ||
| public getInterpreters() { | ||
| if (this.interpreters.length > 0) { | ||
| return Promise.resolve(this.interpreters); |
There was a problem hiding this comment.
No async/await yet in the version of TypeScript that VS Code uses?
There was a problem hiding this comment.
Old habbits die hard, async await is available and used.
I'm using promises as well, in most of these places as we I to maximize parallelism... (i.e. return promises) and await on ALL where possible
| }, [])) | ||
| .then(interpreters => this.interpreters = interpreters); | ||
| } | ||
| } |
| .then(interpreters => _.flatten(interpreters)) | ||
| .then(items => items.map(fixInterpreterDisplayName)) | ||
| .then(items => items.map(fixInterpreterPath)) | ||
| .then(items => items.reduce<PythonInterpreter[]>((prev, current) => { |
There was a problem hiding this comment.
The name prev is misleading since it's a collection and not a single item like current. Maybe rename it accum or accumulator (typical name from functional programming)?
| * However all these environments seem to be listed in the environments.txt file (confirmed on windows and linux) | ||
| * @export | ||
| * @class CondaEnvFileProvider | ||
| * @implements {IInterpreterLocatorService} |
There was a problem hiding this comment.
What are these comments for? They seem to (be trying to) mirror what the code already says so I'm wondering what the benefit is.
There was a problem hiding this comment.
I thought it would be best to write the comments out. Else one might ask whey use environments.txt as well as conda info --env. This makes it obvious that environments.txt can contain additional items. Comment removed.
There was a problem hiding this comment.
Sorry for the ambiguity of that comment, I meant the @export comments and such more than the block comments. (I totally support block comments 😄 )
| * Environments created using the command 'conda create --p python=x.x' are not returned by the above command | ||
| * However all these environments seem to be listed in the environments.txt file (confirmed on windows and linux) | ||
| * @export | ||
| * @class CondaEnvFileProvider |
There was a problem hiding this comment.
This doesn't match the CondaEnvFileService name below.
| }) | ||
| .then(promises => Promise.all(promises)) | ||
| .then(interpreterPaths => { | ||
| return interpreterPaths.filter(item => item.trim().length > 0); |
There was a problem hiding this comment.
Is it worth keeping the paths trimmed instead of tossing the trimmed result? I.e.
interpreterPaths.map(String.prototype.trim).filter(item => item.length > 0);
src/client/interpreter/helpers.ts
Outdated
| if (stdout.length === 0) { | ||
| return ''; | ||
| } | ||
| const lines = stdout.split(/\r?\n/g).filter(line => line.trim().length > 0); |
There was a problem hiding this comment.
Any reason not to keep the trimmed version?
| return this.versionService.getVersion(interpreter, path.basename(interpreter)) | ||
| .then(version => { | ||
| // Strip company name from version | ||
| const startOfCompanyName = version.indexOf(`:: ${AnacondaCompanyName}`); |
There was a problem hiding this comment.
This is going to be tricky once Anaconda Co starts to use that name instead of Continuum Analytics.
There was a problem hiding this comment.
Will add both company names. I'm not too concerned even if it doesn't work as its merely a formatting issue.
environments.txt file returns conda environments not returned by the command
conda info --envs