Labels Milestones

Added password prompt #16

Merged

Spirit-act merged 2 commits from feature-password-prompt into master 6 years ago

Conversation 1 Commits 2 Files Changed 1

Spirit-act commented 6 years ago (Migrated from github.com)

Owner

Resolves #15

Changes:

• Remove the required option on the "password" command line argument
• Added check for None or empty "password" command line argument
• Added option to prompt for a password if command line argument is empty or None

Resolves #15 Changes: - Remove the required option on the "password" command line argument - Added check for None or empty "password" command line argument - Added option to prompt for a password if command line argument is empty or None

connorskees (Migrated from github.com) reviewed 6 years ago

deletefb/deletefb.py

connorskees (Migrated from github.com) commented 6 years ago

Owner

I think you could make this declaration cleaner by just having it as

password = args.password or input('Enter your password: ')

I think you could make this declaration cleaner by just having it as ``` password = args.password or input('Enter your password: ') ```

Spirit-act (Migrated from github.com) reviewed 6 years ago

deletefb/deletefb.py

Spirit-act (Migrated from github.com) commented 6 years ago

Poster

Owner

There is a requirement for a check if the arg is empty.
I could have written it as a oneliner, but I think for beginners it is more understandable.

args_user_password = args.password if args.password is not None or not args.password else input('Enter your password: ')

There is a requirement for a check if the arg is empty. I could have written it as a oneliner, but I think for beginners it is more understandable. `args_user_password = args.password if args.password is not None or not args.password else input('Enter your password: ')`

connorskees (Migrated from github.com) reviewed 6 years ago

deletefb/deletefb.py

connorskees (Migrated from github.com) commented 6 years ago

Owner

args.password is not None or not args.password
Wouldn't this always be true? Not sure if I'm just missing something lol

```args.password is not None or not args.password``` Wouldn't this always be true? Not sure if I'm just missing something lol

weskerfoot (Migrated from github.com) reviewed 6 years ago

deletefb/deletefb.py

weskerfoot (Migrated from github.com) commented 6 years ago

Owner

I'd prefer either:
args.password if args.password else input('Enter your password: ') or args.password or input('Enter your password: ')

Generally it's a good idea in Python to rely on the fact that certain values are "falsey" instead of checking for explicit values like None or False.

Don't want to bikeshed too hard though. You could make it multiline instead of a oneliner too and it doesn't make a huge difference to me.

I'd prefer either: `args.password if args.password else input('Enter your password: ')` or `args.password or input('Enter your password: ')` Generally it's a good idea in Python to rely on the fact that certain values are "falsey" instead of checking for explicit values like None or False. Don't want to bikeshed too hard though. You could make it multiline instead of a oneliner too and it doesn't make a huge difference to me.

Spirit-act (Migrated from github.com) reviewed 6 years ago

deletefb/deletefb.py

Spirit-act (Migrated from github.com) commented 6 years ago

Poster

Owner

But if I use args.password or input('Enter your password: '), the "empty" would not be caught. At least in my tests.

But if I use `args.password or input('Enter your password: ')`, the "empty" would not be caught. At least in my tests.

connorskees (Migrated from github.com) reviewed 6 years ago

deletefb/deletefb.py

connorskees (Migrated from github.com) commented 6 years ago

Owner

@Spirit-act I'm not sure what you mean. I believe empty strings '' evaluate to false?

@Spirit-act I'm not sure what you mean. I believe empty strings `''` evaluate to false?

weskerfoot (Migrated from github.com) reviewed 6 years ago

deletefb/deletefb.py

weskerfoot (Migrated from github.com) commented 6 years ago

Owner

You can also do args.password.strip() if you're worried about whitespace, and it will evaluate to False if the string is made up of whitespace.

You can also do `args.password.strip()` if you're worried about whitespace, and it will evaluate to `False` if the string is made up of whitespace.

Spirit-act (Migrated from github.com) reviewed 6 years ago

deletefb/deletefb.py

Spirit-act (Migrated from github.com) commented 6 years ago

Poster

Owner

unfortunately not, my tests:

$ x = None
$ y = " "
$ a = x or input(': ')
: b
$ a = y or input(': ')
$

unfortunately not, my tests: > $ x = None $ y = " " $ a = x or input(': ') : b $ a = y or input(': ') $

weskerfoot (Migrated from github.com) reviewed 6 years ago

deletefb/deletefb.py

weskerfoot (Migrated from github.com) commented 6 years ago

Owner

>>> x = ""
>>> foo = x or input("blah: ")
blah: dsfsdfdsf

``` >>> x = "" >>> foo = x or input("blah: ") blah: dsfsdfdsf ```

Spirit-act (Migrated from github.com) reviewed 6 years ago

deletefb/deletefb.py

Spirit-act (Migrated from github.com) commented 6 years ago

Poster

Owner

I want to catch your workaround with space:

A workaround at the moment is to prefix your invocation of the command with a space which will cause bash (and maybe other shells?) to not log it in your history.

But if you want, I'll change it to your preferences.

I want to catch your workaround with space: > A workaround at the moment is to prefix your invocation of the command with a space which will cause bash (and maybe other shells?) to not log it in your history. But if you want, I'll change it to your preferences.

weskerfoot (Migrated from github.com) reviewed 6 years ago

deletefb/deletefb.py

weskerfoot (Migrated from github.com) commented 6 years ago

Owner

I don't think we need to worry about that getting caught by this. It's specific to bash and wouldn't be caught by this.

I don't think we need to worry about that getting caught by this. It's specific to bash and wouldn't be caught by this.

Spirit-act (Migrated from github.com) reviewed 6 years ago

Spirit-act (Migrated from github.com) left a comment

• Changed Code
• Pushed new code

deletefb/deletefb.py

Spirit-act (Migrated from github.com) commented 6 years ago

Poster

Owner

Ok, I have changed the code.

Ok, I have changed the code.

weskerfoot commented 6 years ago (Migrated from github.com)

Owner

* Changed Code

* Pushed new code

I'll give it a test run when I get a chance :) Thanks for the contribution

> * Changed Code > > * Pushed new code I'll give it a test run when I get a chance :) Thanks for the contribution

The pull request has been merged as 748bd851e7.

Sign in to join this conversation.

Reviewers

Request review

No reviewers

Labels

Apply labels

Clear labels

bug
Something isn't working dependencies
Pull requests that update a dependency file discussion documentation duplicate
This issue or pull request already exists enhancement
New feature or request good first issue
Good for newcomers help wanted
Extra attention is needed invalid
This doesn't seem right question
Further information is requested wontfix
This will not be worked on

No Label bug dependencies discussion documentation duplicate enhancement good first issue help wanted invalid question wontfix

Milestone

Set milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Assign users

Clear assignees

No Assignees

1 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Write Preview

Loading…

Cancel

Save

There is no content yet.