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

feat(command): add 'toggle' command #1917

Merged
merged 1 commit into from Jan 7, 2021

Conversation

Dentrax
Copy link
Contributor

@Dentrax Dentrax commented Nov 21, 2020

Closes #894

Signed-off-by: Dentrax furkan.turkal@hotmail.com

Description

See the given issue in order to get more description about the toggle feature.

Motivation and Context

As requested at #894 and #1873, this PR adds brand new toggle command.

Screenshots (if appropriate):

asciicast

How Has This Been Tested?

  • Given module name and flag key exist without arg:
$ dasel select -f ~/.config/starship.toml -p toml -m '.kubernetes.disabled' # returns 'true'
$ cargo run toggle kubernetes
$ dasel select -f ~/.config/starship.toml -p toml -m '.kubernetes.disabled' # returns 'false'
  • Given module name and flag key exist with arg:
$ dasel select -f ~/.config/starship.toml -p toml -m '.kubernetes.disabled' # returns 'true'
$ cargo run toggle kubernetes disabled
$ dasel select -f ~/.config/starship.toml -p toml -m '.kubernetes.disabled' # returns 'false'
  • Given module name exist but flag key not exist with arg:
$ cargo run toggle kubernetes show_namespace

See the error: [ERROR] - (starship::configure): Given config key 'show_namespace' must be exist in config file

  • Given module name and non-boolean flag key exist with arg:
$ cargo run toggle kubernetes format

See the error: [ERROR] - (starship::configure): Given config key 'format' must be in 'boolean' format

  • Given module name not exist:
$ cargo run toggle winamp

This will panic-ed, it works the same as the update_configuration() function. We just ignored the None case, by using this: table.get(keys[0]).unwrap(). Shouldn't we cover the case of None? What about adding a new fatal message like: [ERROR] - (starship::configure): Given module 'winamp' not found in config file

  • Given module name and flag key not exist:
$ cargo run toggle winamp volume

Same as above.

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

Some help wanted here too! :)

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

**Happy aliases, you astronauts! **

alias k8s!='cargo run toggle kubernetes'

@andytom andytom requested a review from a team November 23, 2020 18:28
@Dentrax
Copy link
Contributor Author

Dentrax commented Dec 20, 2020

Hey, @andytom, sorry for pinging, any updates about this?

@andytom
Copy link
Member

andytom commented Dec 22, 2020

Hey, @andytom, sorry for pinging, any updates about this?

Hey @Dentrax, sorry I've not managed to give this a proper review yet, I'll try to take asap.

Comment on lines -32 to +36
.value_name("SHELL")
.help(
"The name of the currently running shell\nCurrently supported options: bash, zsh, fish, powershell, ion",
)
.required(true);
.value_name("SHELL")
.help(
"The name of the currently running shell\nCurrently supported options: bash, zsh, fish, powershell, ion",
)
.required(true);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what if anything is changing here, is this just a whitespace change? If so is it required?

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 did not touch here actually, it's so weird that does not show up my name in annotate. Maybe rustfmt did this?

Screen Shot 2020-12-26 at 17 39 11

src/configure.rs Outdated

pub fn toggle_configuration(name: &str, key: &str) {
if let Some(table) = get_configuration().as_table_mut() {
if let Some(values) = table.get(name).unwrap().as_table() {
Copy link
Member

Choose a reason for hiding this comment

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

As you mentioned in the PR description I think it would be better if we handled this a bit more gracefully, maybe we could log an error here if the module section is not set in the config file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! It's done. Now it should log Given module 'winamp' not found in config file and exit(1) in case of error.

Closes starship#894

Signed-off-by: Dentrax <furkan.turkal@hotmail.com>
Copy link
Member

@andytom andytom left a comment

Choose a reason for hiding this comment

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

LGTM

@andytom andytom merged commit f03c3f1 into starship:master Jan 7, 2021
@andytom
Copy link
Member

andytom commented Jan 7, 2021

Thanks for you contribution @Dentrax.

chipbuster pushed a commit to chipbuster/starship that referenced this pull request Jan 9, 2021
Closes starship#894

Signed-off-by: Dentrax <furkan.turkal@hotmail.com>
chipbuster pushed a commit to chipbuster/starship that referenced this pull request Jan 14, 2021
Closes starship#894

Signed-off-by: Dentrax <furkan.turkal@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make individual prompt parts toggleable
2 participants