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

Feature: allow user to disable info fields with flag --disable #82

Merged
merged 6 commits into from
Oct 14, 2019

Conversation

ktsuench
Copy link
Contributor

I implemented the feature using the package Strum to work with the new InfoFields enum. The package allows iteration over enums and creates static str for each value in the enum essentially allowing us to create a vector of selected values from the enum (less code). In this case, I hid a enum value UnrecognizedField as the end user should not see that value in the output of possible values. That value is used to filter out the default value which is an empty string allowing all info fields to be shown.

Use:
Overrides showing the dominant language ascii logo

--disable <disable_field>...         Disable fields to show
                                     Possible values: ["project", "head", "version", "created", "languages",
                                     "authors", "last_change", "repo", "commits", "lines_of_code", "size",
                                     "license"]`

Examples:

  1. Flag used with no value provided, defaults to showing all info

image

  1. Flag used with unrecognized field, CLI errors out

image

  1. Flag used with recognized field, shows all info except that field

image

  1. Flag used with multiple recognized fields in different cases, shows all info except those fields

image

  1. Flag used with languages, shows all info except languages and ASCII logo still shows

image

  1. Flag used with recognized and unrecognized fields, CLI errors out

image

  1. Flag used with all recognized fields, no info shows

image

Addresses issue #73.

ktsuench and others added 3 commits October 14, 2019 00:34
Merge changes from fork parent
if flag specified without value, CLI will now error out
@ktsuench
Copy link
Contributor Author

ktsuench commented Oct 14, 2019

Actually, I realized I could simplify the code a bit more by removing the default value for --disable so that the conditional assignment to disable_fields variable is fully utilized (I don't think the else case would have been reached at all before since there was a default value). Since there's no use for UnrecognizedField value in the enum anymore, I can just use EnumVariantNames from the Strum package which implements variants method on the enum and returns an array of &'static str of values from the enum. The behaviour is still the same as above except case 1 now errors out as shown:

image

@o2sh
Copy link
Owner

o2sh commented Oct 14, 2019

Very Impressive @ktsuench,

Still, I think It would be a better user experience, if the program would not fail if the info name(s) inputed by the user is either missing or not included in the possible values.

  • Case 1: onefetch --disable created head not_a_valid_input repo --> doesn't fail and shows all info names except head created and repo
  • Case 2: onefetch --disable --> doesn't fail and shows all info names
  • Case 3: onefetch --disable not_a_valid_input --> doesn't fail and shows all info names

What do you think ?

@ktsuench
Copy link
Contributor Author

I agree with you on that. Using my first solution we could do that by removing the set of possible values and just have unrecognized values fall back to UnrecognizedField which will get filtered out.

@ktsuench
Copy link
Contributor Author

Reverted my last commit and updated to allow any value but values are checked afterwards.

  1. Flag used with no value provided, defaults to showing all info

image

  1. Flag used with unrecognized field, defaults to showing all info

image

  1. Flag used with recognized and unrecognized fields, ignores unrecognized fields and hides recognized ones

image

src/main.rs Outdated
Comment on lines 60 to 76
if !self.disable_fields.contains(&InfoFields::Project) {
writeln!(
buffer,
"{}{}",
"Project: ".color(color).bold(),
self.project_name
)?;
}

writeln!(
buffer,
"{}{}",
"HEAD: ".color(color).bold(),
self.current_commit
)?;
if !self.disable_fields.contains(&InfoFields::HEAD) {
writeln!(
buffer,
"{}{}",
"HEAD: ".color(color).bold(),
self.current_commit
)?;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Running Vec::contains multiple times is kind of computationally intensive, as each call will iterate through until finding a match. See source code here and here.

Iterating through once and keeping track bool values might be better.
Example:

let mut no_project = false;
// etc.
for field in self.disable_fields.iter() {
    match field {
        InfoFields::Project => no_project = true,
        // etc.
    }
}

if !no_project {
    // write project
}

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 decided to create a struct of bools just to make it easier to track.

@o2sh o2sh merged commit 4d56dbb into o2sh:master Oct 14, 2019
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.

3 participants