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

Implement fuzzy searching into more subcommands #29

Merged
merged 14 commits into from Aug 3, 2017

Conversation

Projects
None yet
2 participants
@yamnikov-oleg
Contributor

yamnikov-oleg commented Aug 1, 2017

Good day! Here is my humble contribution into the project :) Below I describe some of the design decisions I made, which you might want to correct.

  1. 784fd56: Created a function for fuzzy searching named list::search_and_choose_password, which basically combines PasswordStore::search_passwords and list::choose_password_in_list. This function is used throughout all the subcommands.

  2. 784fd56: Changed the behaviour of list::request_password_index_from_stdin (which led to change in behaviour of list::choose_password_in_list): made it return 0-based index of selected password instead of 1-based. I thought it would more intuitive and independent of the UI representation of the passwords list :)

  3. 9075715: Sometimes, when search results only contain one password, rooster asks user to "select password from 1 to 1". This commit makes rooster select the only result quietly without prompting the user.

  4. 4ed9b42: Move help text about fuzzy searching into rooster's usage text.

  5. 6b2efa3: Moved the function you recently created: commands::get::confirm_password_retrieved into the clip module since this code was also repeated in change and regenerate commands. My choice of module might not be the best, but it's the closest one by meaning I could think of :)

Resolves #27.

@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Aug 1, 2017

Owner

Hi @yamnikov-oleg ! Thanks so much for your contribution ! That's awesome ! 👍

I'll have a thorough look at this ASAP and comment again once I have tested it.

Cheers !

Owner

conradkdotcom commented Aug 1, 2017

Hi @yamnikov-oleg ! Thanks so much for your contribution ! That's awesome ! 👍

I'll have a thorough look at this ASAP and comment again once I have tested it.

Cheers !

Show outdated Hide outdated src/list.rs Outdated
@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Aug 2, 2017

Owner

Overall, it all looks very good ! Thank you again for the great work 👍

I'm just going through the code and will post a few comments within the next few minutes 👓

Owner

conradkdotcom commented Aug 2, 2017

Overall, it all looks very good ! Thank you again for the great work 👍

I'm just going through the code and will post a few comments within the next few minutes 👓

@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Aug 2, 2017

Owner

OK @yamnikov-oleg ! I've gone through the code. The password disclosure above is the only thing that needs work. Everything else is perfect ! 😄

Apart from the issue I reported just above about disclosing passwords by mistake, there is just one thing : the generate command does not seem to benefit from the functions you created.

I would love for you to fix both these things because you already did such great work ! 💟

If you don't have more time to work on this (we all have lifes to live! 😃), don't hesitate to let me know and I'll merge your work into master branch and then add a few commits to fix those things.

Owner

conradkdotcom commented Aug 2, 2017

OK @yamnikov-oleg ! I've gone through the code. The password disclosure above is the only thing that needs work. Everything else is perfect ! 😄

Apart from the issue I reported just above about disclosing passwords by mistake, there is just one thing : the generate command does not seem to benefit from the functions you created.

I would love for you to fix both these things because you already did such great work ! 💟

If you don't have more time to work on this (we all have lifes to live! 😃), don't hesitate to let me know and I'll merge your work into master branch and then add a few commits to fix those things.

@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Aug 2, 2017

Owner

My mistake, the generate is not meant to have that behavior. Dummy me.

So it's just the password disclosure thing 😃

Owner

conradkdotcom commented Aug 2, 2017

My mistake, the generate is not meant to have that behavior. Dummy me.

So it's just the password disclosure thing 😃

@yamnikov-oleg

This comment has been minimized.

Show comment
Hide comment
@yamnikov-oleg

yamnikov-oleg Aug 3, 2017

Contributor

Hey, thanks for the compliments :) Your note about accidental password disclosure makes sense. I've updated the password selection function. In the case when there is only one password available to choose it will ask the user to type number 1 to confirm their choice.

"(y/n)" prompt might have been more appropriate, but I didn't feel rightful enough to increase complexity of the UI.

Contributor

yamnikov-oleg commented Aug 3, 2017

Hey, thanks for the compliments :) Your note about accidental password disclosure makes sense. I've updated the password selection function. In the case when there is only one password available to choose it will ask the user to type number 1 to confirm their choice.

"(y/n)" prompt might have been more appropriate, but I didn't feel rightful enough to increase complexity of the UI.

@conradkdotcom conradkdotcom merged commit 3095148 into conradkdotcom:master Aug 3, 2017

@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Aug 3, 2017

Owner

Awesome ! It's good as you did ! Maybe we can tweek the wording a bit in the future. But as is, I think it's good 👍 Thanks again ! I've merged

Owner

conradkdotcom commented Aug 3, 2017

Awesome ! It's good as you did ! Maybe we can tweek the wording a bit in the future. But as is, I think it's good 👍 Thanks again ! I've merged

@conradkdotcom

This comment has been minimized.

Show comment
Hide comment
@conradkdotcom

conradkdotcom Aug 3, 2017

Owner

I will add a few more commits in the coming days and then publish 2.7.0 on crates.io. In the meantime, you can clone the repo, cargo build --release and sudo cp target/release/rooster /usr/bin/rooster if you'd like to use the latest version from git.

Owner

conradkdotcom commented Aug 3, 2017

I will add a few more commits in the coming days and then publish 2.7.0 on crates.io. In the meantime, you can clone the repo, cargo build --release and sudo cp target/release/rooster /usr/bin/rooster if you'd like to use the latest version from git.

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