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.

Sign up for GitHub

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

Jump to bottom

feat: support fnm install --latest #859

Merged
merged 13 commits into from
Nov 18, 2022
Merged

feat: support fnm install --latest #859

merged 13 commits into from
Nov 18, 2022

Conversation

nzhl
Copy link
Contributor

@nzhl nzhl commented Nov 17, 2022 •
edited
Loading

This should help to support install the latest node version using fnm install --latest
Just as #797 expected.

@changeset-bot
Copy link

changeset-bot bot commented Nov 17, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 3e06839

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
fnm Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@nzhl nzhl changed the title feat: support to install the latest version feat: support fnm install --latest Nov 17, 2022
Schniz
Schniz reviewed Nov 18, 2022
View reviewed changes
Copy link
Owner

@Schniz Schniz left a comment

Choose a reason for hiding this comment

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

Can you please run pnpm generate-command-docs to update the commands.md file?

I also had a small suggestion for the error message.

other than that, :lgtm:

I wonder if we can add a test for it somehow. Given that the version is dynamic it might be problematic, but maybe just a smoke test with no expectations other than "not fail"?

src/commands/install.rs Outdated
Comment on lines 178 to 180
#[error("Can't find latest version")]
CantFindLatest,
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should be more explicit about the issue, like: "can't find any versions in the upstream version index"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved, thx for suggestion

@Schniz
Copy link
Owner

Schniz commented Nov 18, 2022

Also, thank you very much for stepping in and contributing! :clap_sunglasses_+1:

can you run cargo fmt and fix cargo clippy issues? :pray_parrot:

@nzhl
Copy link
Contributor Author

nzhl commented Nov 18, 2022

Also, thank you very much for stepping in and contributing! :clap_sunglasses_+1:

can you run cargo fmt and fix cargo clippy issues? :pray_parrot:

Thx for suggestion, i have added smoke test and run cargo fmt and cargo clippy.

@Schniz
Copy link
Owner

Schniz commented Nov 18, 2022

The commands file is not up to date because of the version change :( can you rerun the script?

@nzhl
Copy link
Contributor Author

nzhl commented Nov 18, 2022

The commands file is not up to date because of the version change :( can you rerun the script?

done

@nzhl nzhl force-pushed the feat/support-install-latest branch from 92d49a8 to d17a74f Compare November 18, 2022 14:26
@nzhl
Copy link
Contributor Author

nzhl commented Nov 18, 2022

The commands file is not up to date because of the version change :( can you rerun the script?

sorry, one more question. Do i need to bump version manually ?

@nzhl nzhl force-pushed the feat/support-install-latest branch from d17a74f to 3924dc0 Compare November 18, 2022 15:49
@nzhl nzhl force-pushed the feat/support-install-latest branch from 3924dc0 to 3432ec7 Compare November 18, 2022 16:20
@nzhl nzhl force-pushed the feat/support-install-latest branch from 3432ec7 to 5c5c76f Compare November 18, 2022 16:24
Schniz
Schniz reviewed Nov 18, 2022
View reviewed changes
docs/commands.md Outdated
@@ -1,7 +1,7 @@
# `fnm`

```
fnm 1.32.0
fnm 1.31.1
Copy link
Owner

Choose a reason for hiding this comment

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

I don’t understand why this is not updated, if you’re rebased from main and ran the script. Maybe something is funky there and I’ll fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's updated rn after i run it one more time.

@nzhl nzhl force-pushed the feat/support-install-latest branch from 5c5c76f to cc9e00f Compare November 18, 2022 16:33
@nzhl nzhl force-pushed the feat/support-install-latest branch from cc9e00f to 5c5c76f Compare November 18, 2022 16:34
@nzhl nzhl force-pushed the feat/support-install-latest branch from 5c5c76f to b384755 Compare November 18, 2022 16:35
@Schniz Schniz added PR: New Feature A new feature was added automerge labels Nov 18, 2022
@Schniz
Copy link
Owner

Schniz commented Nov 18, 2022

Amazing! Thank you so much.

@nzhl
Copy link
Contributor Author

nzhl commented Nov 18, 2022 •
edited
Loading

@Schniz Pls approve one more time, seems there are some inconsistency on the usage of node package mangager (pnpm vs yarn) which caused the workflow just fail. I have aligned all workflow to use pnpm, and added command to install pnpm in ci script as well. See

image

@nzhl nzhl force-pushed the feat/support-install-latest branch from 3d35347 to 9a053a7 Compare November 18, 2022 17:18
@Schniz
Remove feat: from changeset
1657e45 
Because GitHub actions approval button is missing
@kodiakhq kodiakhq bot merged commit ca71291 into Schniz:master Nov 18, 2022
@github-actions github-actions bot mentioned this pull request Nov 18, 2022
Merged
@Schniz Schniz mentioned this pull request Nov 19, 2022
Closed
@github-actions github-actions bot mentioned this pull request Nov 20, 2022
Open
@latipun7 latipun7 mentioned this pull request Dec 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Schniz Schniz Schniz left review comments

Assignees
No one assigned
Labels
automerge PR: New Feature A new feature was added
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nzhl @Schniz