Cleanup and fix management commands. #256
Conversation
vil-s
commented
Sep 4, 2017
vil-s
commented
- Ensure that new switch is not created unless the --create flag is passed.
- Add tests to ensure no regression for the --create fix on all commands.
- Add separate positional arguments for commands (instead of "positionals") to improve the --help documentation.
| parser.add_argument( | ||
| '-l', '--list', | ||
| action='store_true', | ||
| dest='list_flags', | ||
| default=False, | ||
| help="List existing samples."), | ||
| help="List existing samples.") |
| parser.add_argument( | ||
| '--everyone', | ||
| action='store_true', | ||
| dest='everyone', | ||
| help="Activate flag for all users."), | ||
| help="Activate flag for all users.") |
| parser.add_argument( | ||
| '--deactivate', | ||
| action='store_false', | ||
| dest='everyone', | ||
| help="Deactivate flag for all users."), | ||
| help="Deactivate flag for all users.") |
| @@ -51,7 +60,7 @@ def handle(self, *args, **options): | |||
| try: | |||
| sample = Sample.objects.get(name=sample_name) | |||
| except Sample.DoesNotExist: | |||
| raise CommandError('This sample does not exist.') | |||
| raise CommandError("This sample doesn't exist.") | |||
There was a problem hiding this comment.
Single quotes. I find "does not" to be more understandable (esp. for non-native English speakers) than "doesn't".
waffle/tests/test_management.py
Outdated
| """ The command shouldn't create a new flag if the create flag is | ||
| not set. | ||
| """ | ||
| with self.assertRaisesRegexp(CommandError, "This flag doesn't exist."): |
There was a problem hiding this comment.
Applicable to all of your new tests:
- Use single quotes to maintain consistency.
- Add an assertion that actually ensures the flag/switch/sample does not exist.
There was a problem hiding this comment.
Fwiw, I strongly prefer double quotes when the string has apostrophes in it, to avoid awkward escape sequences:
'don\'t do this if it can be avoided'
"this's better"
'but this is best'
|
Review changes implemented |
waffle/tests/test_management.py
Outdated
| not set. | ||
| """ | ||
| name = 'test' | ||
| self.assertFalse(Flag.objects.filter(name=name).exists()) |
There was a problem hiding this comment.
The assertions should come after the calls to the management command. The purpose of the test is to ensure the command doesn't create a new instance.
There was a problem hiding this comment.
aha, I misunderstood your previous comment
|
This will be a very nice to have. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@clintonb I fixed the tests and fixed the merge conflicts. It should be ready to go now. |
|
@unklphil please rebase on the master branch, rather than merge master into your own branch. Once that is done, I'll merge. |
|
Just curious why rebasing/squashing in PRs is critical now that github allows squashing as an option in the merge button? |
|
This project using the "Rebase and merge" option solely. That decision for this project wasn't mine, but I agree with it. Merging introduces merge bubbles which are mostly noise. Squashing assumes that every PR should be squashed into a single commit when. The decision to use a single commit is not always applicable. If I happen to fix two separate bugs, they should be two separate commits. In this case, since "Rebase and merge" is the only option available to me, the branch needs to be properly rebased. GitHub will not allow the PR to be merged without a rebase.
|
|
Thank you, I appreciate the detailed response - I've wondered about this, that makes sense now! |
- Ensure that new switch is not created unless the --create flag is passed. - Add tests to ensure no regression for the --create fix on all commands. - Add separate positional arguments for commands (instead of "positionals") to improve the --help documentation.
|
I used the wrong damned option for resolving management.commands.waffle_switch after squashing and rebasing (I was certain that "use mine" would use mine, but it didn't) and did I really don't feel like fixing it, so I'm going to close the issue this PR because it's too much of a hassle for me. |
|
Ugh, I'm sorry, that's the worst! If you haven't deleted the local checkout, it may be possible to recover, if you want—or for a future situation—via the |
|
I'll check tomorrow if it's salvagable |
