Conversation
The project version was updated in the lock file, reflecting a new release iteration. Signed-off-by: Chmouel Boudjnah <chmouel@chmouel.com>
Updated configuration loading to normalize keys by converting hyphens to underscores. This was done to ensure consistency between command-line arguments (which use underscores) and configuration file entries, allowing both hyphenated and underscored keys to be accepted in TOML files. Added tests to verify this behavior. Fixes #47 Signed-off-by: Chmouel Boudjnah <chmouel@chmouel.com>
Summary of ChangesHello @chmouel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant quality-of-life improvement for configuration handling by enabling greater flexibility in how configuration keys are defined. It allows users to interchangeably use hyphens or underscores in their configuration files, which are then consistently normalized internally. This enhancement is thoroughly documented and backed by new test cases, alongside a routine update to the project's version. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a nice quality-of-life improvement by allowing hyphens in configuration file keys, which are then normalized to underscores for internal use. The implementation is straightforward and is accompanied by a good set of new tests. My review includes suggestions to make the code more idiomatic and robust:
- In
src/nextmeeting/cli.py, I've suggested using a dictionary comprehension to simplify the key normalization logic. - In
tests/test_config_loading.py, I've recommended refactoring the tests to use pytest'stmp_pathfixture for cleaner and safer temporary file management.
Overall, this is a solid contribution that improves user experience.
| normalized_config = {} | ||
| for key, value in config_data.items(): | ||
| normalized_key = key.replace("-", "_") | ||
| normalized_config[normalized_key] = value |
There was a problem hiding this comment.
This loop for normalizing keys can be expressed more concisely and idiomatically using a dictionary comprehension. This improves readability by reducing boilerplate code.
| normalized_config = {} | |
| for key, value in config_data.items(): | |
| normalized_key = key.replace("-", "_") | |
| normalized_config[normalized_key] = value | |
| normalized_config = {key.replace("-", "_"): value for key, value in config_data.items()} |
| def test_load_config_normalizes_hyphens_to_underscores(): | ||
| """Test that configuration keys with hyphens are normalized to underscores.""" | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".toml", delete=False) as f: | ||
| f.write("""[nextmeeting] | ||
| caldav-url = "https://example.com/calendar" | ||
| caldav-username = "user" | ||
| caldav-password = "pass" | ||
| max-title-length = 30 | ||
| today-only = true | ||
| """) | ||
| config_path = Path(f.name) | ||
|
|
||
| try: | ||
| config = _load_config(config_path) | ||
|
|
||
| # All keys should be normalized to underscores | ||
| assert config["caldav_url"] == "https://example.com/calendar" | ||
| assert config["caldav_username"] == "user" | ||
| assert config["caldav_password"] == "pass" | ||
| assert config["max_title_length"] == 30 | ||
| assert config["today_only"] is True | ||
|
|
||
| # Hyphens should not exist in the keys | ||
| assert "caldav-url" not in config | ||
| assert "caldav-username" not in config | ||
| assert "caldav-password" not in config | ||
| assert "max-title-length" not in config | ||
| assert "today-only" not in config | ||
| finally: | ||
| os.unlink(config_path) |
There was a problem hiding this comment.
This test can be simplified by using pytest's built-in tmp_path fixture. It automatically creates and cleans up a temporary directory for the test, removing the need for tempfile.NamedTemporaryFile with delete=False and manual os.unlink in a try...finally block. This makes the test cleaner, more readable, and less prone to errors if cleanup fails.
def test_load_config_normalizes_hyphens_to_underscores(tmp_path: Path):
"""Test that configuration keys with hyphens are normalized to underscores."""
config_content = """[nextmeeting]
caldav-url = "https://example.com/calendar"
caldav-username = "user"
caldav-password = "pass"
max-title-length = 30
today-only = true
"""
config_path = tmp_path / "config.toml"
config_path.write_text(config_content)
config = _load_config(config_path)
# All keys should be normalized to underscores
assert config["caldav_url"] == "https://example.com/calendar"
assert config["caldav_username"] == "user"
assert config["caldav_password"] == "pass"
assert config["max_title_length"] == 30
assert config["today_only"] is True
# Hyphens should not exist in the keys
assert "caldav-url" not in config
assert "caldav-username" not in config
assert "caldav-password" not in config
assert "max-title-length" not in config
assert "today-only" not in config| def test_load_config_works_with_both_formats(): | ||
| """Test that both hyphen and underscore formats work in config files.""" | ||
| # Test with hyphens (README format) | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".toml", delete=False) as f: | ||
| f.write("""[nextmeeting] | ||
| caldav-url = "https://example.com/hyphens" | ||
| caldav-username = "user-hyphens" | ||
| """) | ||
| config_path_hyphens = Path(f.name) | ||
|
|
||
| # Test with underscores (internal format) | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".toml", delete=False) as f: | ||
| f.write("""[nextmeeting] | ||
| caldav_url = "https://example.com/underscores" | ||
| caldav_username = "user_underscores" | ||
| """) | ||
| config_path_underscores = Path(f.name) | ||
|
|
||
| try: | ||
| # Both should work and produce the same key format | ||
| config_hyphens = _load_config(config_path_hyphens) | ||
| config_underscores = _load_config(config_path_underscores) | ||
|
|
||
| # Both should have normalized underscore keys | ||
| assert "caldav_url" in config_hyphens | ||
| assert "caldav_username" in config_hyphens | ||
| assert "caldav_url" in config_underscores | ||
| assert "caldav_username" in config_underscores | ||
|
|
||
| # Values should be preserved | ||
| assert config_hyphens["caldav_url"] == "https://example.com/hyphens" | ||
| assert config_hyphens["caldav_username"] == "user-hyphens" | ||
| assert config_underscores["caldav_url"] == "https://example.com/underscores" | ||
| assert config_underscores["caldav_username"] == "user_underscores" | ||
|
|
||
| finally: | ||
| os.unlink(config_path_hyphens) | ||
| os.unlink(config_path_underscores) |
There was a problem hiding this comment.
Similar to the previous test, this can be simplified using the tmp_path fixture to manage temporary files. This avoids manual file creation and cleanup, making the test more robust and easier to read.
def test_load_config_works_with_both_formats(tmp_path: Path):
"""Test that both hyphen and underscore formats work in config files."""
# Test with hyphens (README format)
config_content_hyphens = """[nextmeeting]
caldav-url = "https://example.com/hyphens"
caldav-username = "user-hyphens"
"""
config_path_hyphens = tmp_path / "config_hyphens.toml"
config_path_hyphens.write_text(config_content_hyphens)
# Test with underscores (internal format)
config_content_underscores = """[nextmeeting]
caldav_url = "https://example.com/underscores"
caldav_username = "user_underscores"
"""
config_path_underscores = tmp_path / "config_underscores.toml"
config_path_underscores.write_text(config_content_underscores)
# Both should work and produce the same key format
config_hyphens = _load_config(config_path_hyphens)
config_underscores = _load_config(config_path_underscores)
# Both should have normalized underscore keys
assert "caldav_url" in config_hyphens
assert "caldav_username" in config_hyphens
assert "caldav_url" in config_underscores
assert "caldav_username" in config_underscores
# Values should be preserved
assert config_hyphens["caldav_url"] == "https://example.com/hyphens"
assert config_hyphens["caldav_username"] == "user-hyphens"
assert config_underscores["caldav_url"] == "https://example.com/underscores"
assert config_underscores["caldav_username"] == "user_underscores"| def test_parse_args_accepts_config_with_hyphens(): | ||
| """Test that parse_args properly handles config files with hyphenated keys.""" | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".toml", delete=False) as f: | ||
| f.write("""[nextmeeting] | ||
| caldav-url = "https://example.com/config-test" | ||
| caldav-username = "config-user" | ||
| max-title-length = 25 | ||
| """) | ||
| config_path = f.name | ||
|
|
||
| # Clear environment variables that might interfere | ||
| with patch.dict(os.environ, {}, clear=True): | ||
| original_argv = sys.argv | ||
| try: | ||
| sys.argv = ["nextmeeting", "--config", config_path] | ||
| args = parse_args() | ||
|
|
||
| # The arguments should be accessible with underscores | ||
| assert getattr(args, "caldav_url") == "https://example.com/config-test" | ||
| assert getattr(args, "caldav_username") == "config-user" | ||
| assert getattr(args, "max_title_length") == 25 | ||
|
|
||
| finally: | ||
| sys.argv = original_argv | ||
| os.unlink(config_path) |
There was a problem hiding this comment.
This test can also be refactored to use the tmp_path fixture. This simplifies the test setup by handling temporary file management automatically, which is the standard practice in pytest.
def test_parse_args_accepts_config_with_hyphens(tmp_path: Path):
"""Test that parse_args properly handles config files with hyphenated keys."""
config_content = """[nextmeeting]
caldav-url = "https://example.com/config-test"
caldav-username = "config-user"
max-title-length = 25
"""
config_path = tmp_path / "config.toml"
config_path.write_text(config_content)
# Clear environment variables that might interfere
with patch.dict(os.environ, {}, clear=True):
original_argv = sys.argv
try:
sys.argv = ["nextmeeting", "--config", str(config_path)]
args = parse_args()
# The arguments should be accessible with underscores
assert getattr(args, "caldav_url") == "https://example.com/config-test"
assert getattr(args, "caldav_username") == "config-user"
assert getattr(args, "max_title_length") == 25
finally:
sys.argv = original_argv
chmouel
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, making the configuration more user-friendly by accepting both hyphens and underscores. The addition of comprehensive tests is also excellent. My feedback focuses on a couple of areas:
- In
src/nextmeeting/cli.py, I've suggested a more concise way to implement the key normalization using a dictionary comprehension. - In the new test file
tests/test_config_loading.py, I've provided suggestions to refactor the tests to use standardpytestfeatures like thetmp_pathfixture and test parametrization. This will make the tests cleaner, more robust, and easier to maintain.
| normalized_config = {} | ||
| for key, value in config_data.items(): | ||
| normalized_key = key.replace("-", "_") | ||
| normalized_config[normalized_key] = value | ||
|
|
||
| return normalized_config |
There was a problem hiding this comment.
The key normalization logic can be expressed more concisely and idiomatically using a dictionary comprehension. This is a common Python pattern for creating a new dictionary based on an existing iterable.
| normalized_config = {} | |
| for key, value in config_data.items(): | |
| normalized_key = key.replace("-", "_") | |
| normalized_config[normalized_key] = value | |
| return normalized_config | |
| return {key.replace("-", "_"): value for key, value in config_data.items()} |
| def test_load_config_normalizes_hyphens_to_underscores(): | ||
| """Test that configuration keys with hyphens are normalized to underscores.""" | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".toml", delete=False) as f: | ||
| f.write("""[nextmeeting] | ||
| caldav-url = "https://example.com/calendar" | ||
| caldav-username = "user" | ||
| caldav-password = "pass" | ||
| max-title-length = 30 | ||
| today-only = true | ||
| """) | ||
| config_path = Path(f.name) | ||
|
|
||
| try: | ||
| config = _load_config(config_path) | ||
|
|
||
| # All keys should be normalized to underscores | ||
| assert config["caldav_url"] == "https://example.com/calendar" | ||
| assert config["caldav_username"] == "user" | ||
| assert config["caldav_password"] == "pass" | ||
| assert config["max_title_length"] == 30 | ||
| assert config["today_only"] is True | ||
|
|
||
| # Hyphens should not exist in the keys | ||
| assert "caldav-url" not in config | ||
| assert "caldav-username" not in config | ||
| assert "caldav-password" not in config | ||
| assert "max-title-length" not in config | ||
| assert "today-only" not in config | ||
| finally: | ||
| os.unlink(config_path) |
There was a problem hiding this comment.
For cleaner and more idiomatic pytest tests, consider using the tmp_path fixture for creating temporary files. It automatically handles cleanup, removing the need for try...finally blocks and manual os.unlink calls. You will need to add import pytest at the top of the file and change the function signature to accept the tmp_path fixture. This pattern can be applied to the other tests in this file as well.
def test_load_config_normalizes_hyphens_to_underscores(tmp_path: Path):
"""Test that configuration keys with hyphens are normalized to underscores."""
config_path = tmp_path / "config.toml"
config_path.write_text("""[nextmeeting]
caldav-url = "https://example.com/calendar"
caldav-username = "user"
caldav-password = "pass"
max-title-length = 30
today-only = true
""")
config = _load_config(config_path)
# All keys should be normalized to underscores
assert config["caldav_url"] == "https://example.com/calendar"
assert config["caldav_username"] == "user"
assert config["caldav_password"] == "pass"
assert config["max_title_length"] == 30
assert config["today_only"] is True
# Hyphens should not exist in the keys
assert "caldav-url" not in config
assert "caldav-username" not in config
assert "caldav-password" not in config
assert "max-title-length" not in config
assert "today-only" not in config| def test_load_config_works_with_both_formats(): | ||
| """Test that both hyphen and underscore formats work in config files.""" | ||
| # Test with hyphens (README format) | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".toml", delete=False) as f: | ||
| f.write("""[nextmeeting] | ||
| caldav-url = "https://example.com/hyphens" | ||
| caldav-username = "user-hyphens" | ||
| """) | ||
| config_path_hyphens = Path(f.name) | ||
|
|
||
| # Test with underscores (internal format) | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".toml", delete=False) as f: | ||
| f.write("""[nextmeeting] | ||
| caldav_url = "https://example.com/underscores" | ||
| caldav_username = "user_underscores" | ||
| """) | ||
| config_path_underscores = Path(f.name) | ||
|
|
||
| try: | ||
| # Both should work and produce the same key format | ||
| config_hyphens = _load_config(config_path_hyphens) | ||
| config_underscores = _load_config(config_path_underscores) | ||
|
|
||
| # Both should have normalized underscore keys | ||
| assert "caldav_url" in config_hyphens | ||
| assert "caldav_username" in config_hyphens | ||
| assert "caldav_url" in config_underscores | ||
| assert "caldav_username" in config_underscores | ||
|
|
||
| # Values should be preserved | ||
| assert config_hyphens["caldav_url"] == "https://example.com/hyphens" | ||
| assert config_hyphens["caldav_username"] == "user-hyphens" | ||
| assert config_underscores["caldav_url"] == "https://example.com/underscores" | ||
| assert config_underscores["caldav_username"] == "user_underscores" | ||
|
|
||
| finally: | ||
| os.unlink(config_path_hyphens) | ||
| os.unlink(config_path_underscores) |
There was a problem hiding this comment.
This test can be made more concise and maintainable by using pytest.mark.parametrize. This avoids duplicating the test logic for different inputs (hyphens vs. underscores) and makes it easier to add more test cases in the future. This also uses the tmp_path fixture for cleaner file handling. You will need to add import pytest at the top of the file.
@pytest.mark.parametrize(
("config_content", "expected_values"),
[
(
"""[nextmeeting]
caldav-url = "https://example.com/hyphens"
caldav-username = "user-hyphens"
""",
{
"caldav_url": "https://example.com/hyphens",
"caldav_username": "user-hyphens",
},
),
(
"""[nextmeeting]
caldav_url = "https://example.com/underscores"
caldav_username = "user_underscores"
""",
{
"caldav_url": "https://example.com/underscores",
"caldav_username": "user_underscores",
},
),
],
)
def test_load_config_works_with_both_formats(tmp_path: Path, config_content: str, expected_values: dict):
"""Test that both hyphen and underscore formats work in config files."""
config_path = tmp_path / "config.toml"
config_path.write_text(config_content)
config = _load_config(config_path)
assert config == expected_values| def test_parse_args_accepts_config_with_hyphens(): | ||
| """Test that parse_args properly handles config files with hyphenated keys.""" | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".toml", delete=False) as f: | ||
| f.write("""[nextmeeting] | ||
| caldav-url = "https://example.com/config-test" | ||
| caldav-username = "config-user" | ||
| max-title-length = 25 | ||
| """) | ||
| config_path = f.name | ||
|
|
||
| # Clear environment variables that might interfere | ||
| with patch.dict(os.environ, {}, clear=True): | ||
| original_argv = sys.argv | ||
| try: | ||
| sys.argv = ["nextmeeting", "--config", config_path] | ||
| args = parse_args() | ||
|
|
||
| # The arguments should be accessible with underscores | ||
| assert getattr(args, "caldav_url") == "https://example.com/config-test" | ||
| assert getattr(args, "caldav_username") == "config-user" | ||
| assert getattr(args, "max_title_length") == 25 | ||
|
|
||
| finally: | ||
| sys.argv = original_argv | ||
| os.unlink(config_path) |
There was a problem hiding this comment.
Similarly to the other tests in this file, using the tmp_path fixture here will simplify the code by removing manual file creation and cleanup. Also, using direct attribute access (args.caldav_url) can be slightly more readable than getattr(args, 'caldav_url') when the attribute name is known.
def test_parse_args_accepts_config_with_hyphens(tmp_path: Path):
"""Test that parse_args properly handles config files with hyphenated keys."""
config_path = tmp_path / "config.toml"
config_path.write_text("""[nextmeeting]
caldav-url = "https://example.com/config-test"
caldav-username = "config-user"
max-title-length = 25
""")
# Clear environment variables that might interfere
with patch.dict(os.environ, {}, clear=True):
original_argv = sys.argv
try:
sys.argv = ["nextmeeting", "--config", str(config_path)]
args = parse_args()
# The arguments should be accessible with underscores
assert args.caldav_url == "https://example.com/config-test"
assert args.caldav_username == "config-user"
assert args.max_title_length == 25
finally:
sys.argv = original_argvUsed a dictionary comprehension for normalizing configuration keys during loading. This simplified the creation of the normalized configuration dictionary.
1 participant
This pull request improves configuration file handling in the
nextmeetingCLI by ensuring that both hyphens and underscores are accepted in configuration keys, making the tool more flexible and user-friendly. It also adds comprehensive tests to verify this normalization logic and updates the documentation to reflect the change.Configuration normalization improvements:
_load_configinsrc/nextmeeting/cli.pyto normalize configuration keys by converting hyphens to underscores, ensuring compatibility with both formats and matching argparse behavior.Testing enhancements:
tests/test_config_loading.pyto verify that configuration keys with hyphens are correctly normalized to underscores, that both formats work interchangeably, and thatparse_argsproperly handles config files with hyphenated keys.Documentation update:
README.mdto clarify that both hyphens and underscores are accepted in configuration keys.