Skip to content

fixed #1311#1417

Merged
chriskrycho merged 4 commits intovolta-cli:mainfrom
gautamprikshit1:info-installed-change
Dec 29, 2023
Merged

fixed #1311#1417
chriskrycho merged 4 commits intovolta-cli:mainfrom
gautamprikshit1:info-installed-change

Conversation

@gautamprikshit1
Copy link
Copy Markdown
Contributor

@gautamprikshit1 gautamprikshit1 commented Jan 11, 2023

Fixes #1311

Copy link
Copy Markdown
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Thanks! Needs a small tweak to work as suggested in #1311, suggested as a revision.

#[inline]
fn info_installed<T: Display + Sized>(tool: T) {
info!("{} installed and set {} as default", success_prefix(), tool);
info!("{} installed and set {} as default\n{} to use {tool} in this project run `volta pin {too;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like this is missing some of the necessary bits to actually present the right message here!

Suggested change
info!("{} installed and set {} as default\n{} to use {tool} in this project run `volta pin {too;
info!("{} installed and set {} as default", success_prefix(), tool);
info!("{} to use {} in this project run `volta pin {}`, success_prefix(), tool);

Copy link
Copy Markdown
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Thanks! I've kicked off CI (which should pass) and we can get this merged.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make it clear that pin is needed in addition to npm i npm

2 participants