Skip to content
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

Fix parsing of command line parameter overrides using whitespace #38

Merged
merged 2 commits into from
Sep 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,18 @@ All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](https://semver.org).

## [0.7.1] - 2022-09-28

### Added
### Changed
### Deprecated
### Removed
### Fixed
- Incorrectly parsed command line parameter for overrides using whitespace (`--key value`)

### Security


## [0.7.0] - 2022-08-26

### Added
Expand Down
4 changes: 2 additions & 2 deletions fromconfig/cli/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ def _no_op(*_args, **_kwargs):

argv = sys.argv[1:]
fire.Fire(_parse_args, argv)
num_args_used = len(_paths) + len(_overrides) + 1 # +1 for the fire separator
command = " ".join(argv[num_args_used:])
idx_of_fire_separator = len(argv) if "-" not in argv else argv.index("-")
command = " ".join(argv[idx_of_fire_separator + 1 :])
return _paths, _overrides, command


Expand Down
2 changes: 1 addition & 1 deletion fromconfig/version.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# pylint: disable=all

__version__ = "0.7.0"
__version__ = "0.7.1"
__author__ = "Criteo"

__major__ = __version__.split(".")[0]
Expand Down
38 changes: 37 additions & 1 deletion tests/unit/cli/test_cli_main.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,53 @@
"""Tests for cli.main."""

import fromconfig
from fromconfig.cli.main import parse_args
import sys
from unittest.mock import patch

import subprocess


def capture(command):
"""Utility to execute and capture the result of a commmand."""
"""Utility to execute and capture the result of a command."""
proc = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
out, err = proc.communicate()
return out, err, proc.returncode


def test_cli_parse_args():
"""Test cli.parse_args."""
argv = ["fromconfig", "config.yaml", "params.yaml", "-", "model", "-", "train"]
with patch.object(sys, "argv", argv):
paths, overrides, command = parse_args()
expected_paths = ["config.yaml", "params.yaml"]
expected_overrides = {}
expected_command = "model - train"
assert paths == expected_paths
assert overrides == expected_overrides
assert command == expected_command

argv = ["fromconfig", "config.yaml", "--output", "/tmp", "-", "run"]
with patch.object(sys, "argv", argv):
paths, overrides, command = parse_args()
expected_paths = ["config.yaml"]
expected_overrides = {"output": "/tmp"}
expected_command = "run"
assert paths == expected_paths
assert overrides == expected_overrides
assert command == expected_command

argv = ["fromconfig", "config.yaml", "--output=/tmp", "-", "run"]
with patch.object(sys, "argv", argv):
paths, overrides, command = parse_args()
expected_paths = ["config.yaml"]
expected_overrides = {"output": "/tmp"}
expected_command = "run"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could be worth adding a unit test that covers the case of first example in the doc (https://fromconfig.github.io/#/). In such a case there are two "-".
I don't see why your code would not work in such setting but it is probably worth being sure that doc won't break.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! The test case is added by the commit below.

assert paths == expected_paths
assert overrides == expected_overrides
assert command == expected_command


def test_cli_main_nothing():
"""Test that fromconfig with no argument does not error."""
out, err, exitcode = capture(["fromconfig"])
Expand Down