-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add -f flag as option on rm. #1007
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
Conversation
044e092 to
5504ef4
Compare
|
Please give me a ping if this is accepted so that I can add the flag to bash completion. |
compose/cli/main.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? I believe docopt normalises -f to --force.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are entirely right. I seem to have fixed a bug that was fixed! Sorry, I also tested this against my current version of fig where the flag failed and thought the above had fixed it. I can close this pull request if that makes the most sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need. You can make a change while keeping it to one commit with git commit --amend, and then push that to your branch with git push --force.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear: it's your change to the docs for rm that causes -f to work as a short version of --force. That's how docopt works - you write the docs and it implements the flags.
But we don't need the check for options.get('-f') because docopt is smart and just sets options['--force'] if it sees -f. So this line can be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok that's awesome. Have amended the commit.
|
LGTM |
|
LGTM, but it seems like the wercker build hasn't run against this PR for some reason? |
Signed-off-by: Kingsley Kelly <kingsley.kelly@gmail.com>
|
I tried amending the commit and opening a new pull request, neither of which triggered wercker. I'm unfamiliar with the service, is this likely to be an error on my end? |
|
Nope, I don't think it's on your end. I know @aanand is working on having the test suite run on jenkins, I'm not sure if wercker integration was disabled, or if there is an error on their end. |
|
Looks like Wercker is still running but we've lost the indicator, possibly because I added hooks for Jenkins and it's confusing GitHub about what to show. The build was successful, so I'm going to merge. |
Add -f flag as option on rm. Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
This fixes #752
Just added another flag to rm and a test. I'm not sure if this is addressed by any other pull requests, but I couldn't find them.