This branch is really good. These tests will help us quite a bit while we
implement the LEP. Here are my thoughts from reading through the diff:
The combination of the negative test for step_name ("not in steps_to_skip") and
the ternary operator in _include_step is really hard for me to understand.
Maybe this instead:
def _include_step(self, step_name, namespace):
"""Return True if the given *step_name* must be run, False otherwise.
A step is included in the command execution if the step included in
`--steps` (or steps is None) and is not included in `--skip-steps`.
"""
steps_to_skip = namespace.skip_steps or []
steps_to_run = namespace.steps
# If explicitly told to skip a step, then skip it.
if step_name in steps_to_skip:
return False
# If no list of steps was provided then any non-skipped are to be run.
if steps_to_run is None:
return True
# A list of steps to run was provided, so the step has to be included in
# order to be run.
return step_name in steps_to_run
I am not entirely sure the above is right, which bolsters my feeling that the
original was hard to understand.
Since the subcommand tests always have a get_arguments function which is called
and assigned to expected_arguments, it might be slightly better to just have a
required get_arguments method instead.
I don't feel strongly about it, but for the sake of diversity: here is a
version of get_random_string that is a bit shorter and more direct:
def get_random_string(size=10):
"""Return a random string to be used in tests."""
return ''.join(random.sample(string.ascii_letters, size))
I really like the explanation in the StepsBasedSubCommandTestMixin docstring of
how to use the class. I wonder if it would be good to leave the "must define"
attributes undefined so AttributeErrors are generated if a subclass leaves them
out.
This branch is really good. These tests will help us quite a bit while we
implement the LEP. Here are my thoughts from reading through the diff:
The combination of the negative test for step_name ("not in steps_to_skip") and
the ternary operator in _include_step is really hard for me to understand.
Maybe this instead:
def _include_step(self, step_name, namespace):
"""Return True if the given *step_name* must be run, False otherwise.
A step is included in the command execution if the step included in skip_steps or []
`--steps` (or steps is None) and is not included in `--skip-steps`.
"""
steps_to_skip = namespace.
steps_to_run = namespace.steps
# If explicitly told to skip a step, then skip it.
if step_name in steps_to_skip:
return False
# If no list of steps was provided then any non-skipped are to be run.
if steps_to_run is None:
return True
# A list of steps to run was provided, so the step has to be included in
# order to be run.
return step_name in steps_to_run
I am not entirely sure the above is right, which bolsters my feeling that the
original was hard to understand.
Since the subcommand tests always have a get_arguments function which is called
and assigned to expected_arguments, it might be slightly better to just have a
required get_arguments method instead.
I don't feel strongly about it, but for the sake of diversity: here is a
version of get_random_string that is a bit shorter and more direct:
def get_random_ string( size=10) : random. sample( string. ascii_letters, size))
"""Return a random string to be used in tests."""
return ''.join(
I really like the explanation in the StepsBasedSubCo mmandTestMixin docstring of
how to use the class. I wonder if it would be good to leave the "must define"
attributes undefined so AttributeErrors are generated if a subclass leaves them
out.