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

Added json option to env command #800

Merged
merged 21 commits into from
Nov 17, 2022
Merged

Conversation

zaucy
Copy link
Contributor

@zaucy zaucy commented Aug 17, 2022

Main motivation for this PR is to make the nushell experience better (#463). Taken from one of the suggestions in the comments I added a --json option.

fnm env --json | from json | load-env
let-env PATH = ($env.PATH | prepend ($env.FNM_MULTISHELL_PATH + "/bin"))

@changeset-bot
Copy link

changeset-bot bot commented Aug 17, 2022

🦋 Changeset detected

Latest commit: 456093f

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

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.

I'm having some refactoring questions/suggestions here that I would love to know your input on. But on the other side, I wonder: should we have a support for --shell=nu[shell]? the json implementation can allow implementing this on userland easier, but maybe we should bake this knowledge into fnm and test it properly to make other features like use-on-cd work?

Comment on lines 18 to 20
/// Print JSON instead of shell commands.
#[clap(long)]
json: bool,
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably make it broader, so --output=json is preferable. --output=json conflicts with --shell=ANY which can be declared with #[clap(conflicts_with = "shell")]

That being said, --shell=... is practically --output=..., right? so maybe we can leverage --shell= to be used as --shell=json? Kinda ugly naming but does not break and does not introduce another API complexity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I didn't know about conflicts_with. Thanks!

Haha I almost did --shell=json. Maybe deprecating --shell and replace it with --output? That reads well in my head.

fnm env --output=bash

Then for compatbility sake just have --shell map to --output? Is there an option for that in clap?

src/commands/env.rs Outdated Show resolved Hide resolved
src/commands/env.rs Outdated Show resolved Hide resolved
src/commands/env.rs Outdated Show resolved Hide resolved
@Schniz
Copy link
Owner

Schniz commented Aug 17, 2022

ah and by the way, thanks for stepping up and making a PR. I really appreciate your contribution. It's not something I take for granted 🙏

@zaucy
Copy link
Contributor Author

zaucy commented Aug 17, 2022

Thanks for the review and improvement suggestions! I'll be honest my decision making was mainly driven by my lack of experience with rust haha.

I 100% agree adding nushell to the --shell option would be much better. I initially went down that route, but got a little stuck with functions such as to_clap_shell since nushell wasn't part of the result enum. I figured the json way was less intrusive in the codebase as a first step considering my lack of rust experience.

@Schniz
Copy link
Owner

Schniz commented Aug 17, 2022

re: to_clap_shell:

(off topic, I'm using lists on github any time I want to link to an issue just to have the nice formatting they do on lists. FORMATTING ISSUES EVERYWHERE IN GITHUB WEN)

@Schniz
Copy link
Owner

Schniz commented Aug 17, 2022

my decision making was mainly driven by my lack of experience with rust

I built fnm in Rust when I had zero experience in Rust, I want to refactor so much of the current code but I have no time 😜 you're doing good 🙏

@Schniz
Copy link
Owner

Schniz commented Nov 16, 2022

@zaucy I took the liberty to fix conflicts so I'll merge this finally. Sorry it took so long. :pray_parrot:

@zaucy
Copy link
Contributor Author

zaucy commented Nov 16, 2022

Ayy! Love that

@Schniz Schniz added PR: New Feature A new feature was added automerge labels Nov 16, 2022
in Windows PowerShell, seems like .ps1 is mandatory, otherwise it does not run the script.

In Windows PowerShell, it outputs ASCII by default. This commit makes it output UTF8 without BOM
when redirecting output.
@Schniz Schniz merged commit 405b987 into Schniz:master Nov 17, 2022
@github-actions github-actions bot mentioned this pull request Nov 17, 2022
@zaucy zaucy deleted the feat/json-env-command branch November 17, 2022 23:15
nzhl pushed a commit to nzhl/fnm that referenced this pull request Nov 18, 2022
@remmycat remmycat mentioned this pull request Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge PR: New Feature A new feature was added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants