Skip to content
This repository was archived by the owner on Mar 21, 2023. It is now read-only.

feature: Detect coursier if it's available locally - #524

Open
tgodzik wants to merge 1 commit into
scalameta:mainfrom
tgodzik:find-coursier
Open

feature: Detect coursier if it's available locally#524
tgodzik wants to merge 1 commit into
scalameta:mainfrom
tgodzik:find-coursier

Conversation

@tgodzik

@tgodzik tgodzik commented Dec 7, 2022

Copy link
Copy Markdown
Contributor

This should help with cases, where downloading coursier automatically is problematic.

Next step would be to also download it automatically when needed.

This should help with cases, where downloading coursier automatically is problematic.

Next step would be to also download it automatically when needed.
@tgodzik
tgodzik requested review from ckipp01 and kpodsiad December 7, 2022 12:59

@ckipp01 ckipp01 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So I see there is a fetchMetals.test.ts, but that only is looking at versions. There aren't really any tests that ensure this works. Do you think it'd make sense to try and add some?

I don't have any strong feelings about this btw, since coc-metals is deprecated, the VS Code extension is really the only thing using this now. If it's easier this can all be inlined if need be as well.

Comment thread src/fetchMetals.ts
} else return [p];
})
.find(
(p) => p.endsWith(path.sep + "cs") || p.endsWith(path.sep + "coursier")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's probably not a huge deal, but I had a lot of problems with this in nvim-metals when trying to find cs on windows. I think this will just pass over and default to coursier which is fine, but if you did also want to check for windows and a cs.bat I do that here. Again, I don't think we need to, but looking for cs on windows won't work afaik.

Comment thread src/fetchMetals.ts
"-p",
];

let possibleCoursier = process.env["PATH"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let -> const

Comment thread src/fetchMetals.ts
Comment on lines +75 to +87
if (possibleCoursier) {
let coursier: string = possibleCoursier;
console.debug(`Using coursier located at ${coursier}`);
output.appendLine(`Using coursier located at ${coursier}`);
spawn(coursier, ["version"])
.then((_out) => spawn(coursier, coursierArgs))
.catch((err: Error) => {
output.appendLine(err.message);
console.debug(err);
return spawnDefault();
});
}
return spawnDefault();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

right now this won't work at all because if branch doesn't have return statement so spawn(coursier, ["version"]) will be discarded and spawnDefault promise will be returned.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants