Added CLI command for setting configuration. #5
Conversation
eriq-augustine
left a comment
eriq-augustine
left a comment
There was a problem hiding this comment.
A pretty good start.
There are some design issues that are making things awkward.
Remember if things are awkward or start to smell,
it could indicate a design issue.
edq/testing/cli.py
Outdated
| if (cwd is not None): | ||
| self.cwd = self._expand_paths(cwd) |
There was a problem hiding this comment.
We usually modify init inputs before setting them as members.
So do modification before setting self.cwd.
eriq-augustine
left a comment
eriq-augustine
left a comment
There was a problem hiding this comment.
Architecture is looking better.
| @@ -0,0 +1,81 @@ | |||
| """ | |||
| Set a configuration option. | |||
There was a problem hiding this comment.
Comment is incorrect.
| def run_cli(args: argparse.Namespace) -> int: | ||
| """ Run the CLI. """ | ||
|
|
||
| config: typing.Dict[str, str] = {} |
There was a problem hiding this comment.
Name needs more context.
config is a bad name because it does not contain current configuration options.
config_to_set (as you have in the args) is fine.
| (key, value) = edq.core.config._parse_cli_config_option(config_option) | ||
| config[key] = value | ||
|
|
||
| # Defaults to the local configuration if no configuration type is specified. |
| if (args.write_local): | ||
| local_config_path = args._config_params[edq.core.config.LOCAL_CONFIG_PATH_KEY] | ||
| if (local_config_path is None): | ||
| local_config_path = args._config_params[edq.core.config.FILENAME_KEY] |
There was a problem hiding this comment.
I would put a newline after this.
| edq.core.config.write_config_to_file(global_config_path, config) | ||
| elif (args.write_file_path is not None): | ||
| edq.core.config.write_config_to_file(args.write_file_path, config) | ||
|
|
There was a problem hiding this comment.
Add an else and raise an exception.
We usually do this with things like this and enums to protect us in the future if someone adds a new possible value.
Like if we decided to add system-level config, then an else would protect us.
| def _parse_cli_config_option( | ||
| config_option: str, | ||
| ) -> typing.Tuple[str, str]: | ||
| """ Parse and validate a CLI configuration option string, returs the resulting config option as a key value pair. """ |
| def _parse_cli_config_option( | ||
| config_option: str, | ||
| ) -> typing.Tuple[str, str]: | ||
| """ Parse and validate a CLI configuration option string, returs the resulting config option as a key value pair. """ |
There was a problem hiding this comment.
Mention in this comment what the values you are parsing look like.
| return config, sources | ||
| return config, sources, config_params | ||
|
|
||
| def _parse_cli_config_option( |
There was a problem hiding this comment.
I would move away from saying these are "CLI" options,
and instead refer to them as string options.
(Since non-CLI code is allowed to call this.)
Comments and exceptions will need to be updated to reflect this.
| return key, value | ||
|
|
||
| def _validate_config_key(config_key: str, config_value: str) -> str: | ||
| """ Validate a configuration key and return its stripped version. """ |
There was a problem hiding this comment.
s/stripped/clean/
You may want to do other cleaning in the future.
| self.work_dir: str = os.getcwd() | ||
| """ The directory the test runs from. """ | ||
|
|
||
| if (work_dir is not None): | ||
| work_dir_expanded = self._expand_paths(work_dir) | ||
| self.work_dir = work_dir_expanded |
There was a problem hiding this comment.
!
A relevant comment was marked resolved, but not handled.
Comment is outdated, but now unresolved.
2 participants
This PR adds a new CLI command for setting configuration options, along with the related tests.
It also improves the CLI testing infrastructure by allowing developers to specify the working directory (cwd) for their CLI tests.