-
Notifications
You must be signed in to change notification settings - Fork 7
Fix regex to correctly capture default values in environment variable replacement #38
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the regex pattern used for capturing default values in environment variable replacement and adds a comprehensive suite of tests for various default value scenarios.
- Updated the regex to restrict the default value pattern and added escaping for special characters.
- Added new test cases for environment variable default values including nested defaults and special characters (asterisk, plus, question mark, and right brace).
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tests/environment_variable_evaluation/test_environment_variables_braces.py | Added tests for multiple default scenarios including nested defaults and special characters |
| src/pycomposefile/compose_element/compose_datatype_transformer.py | Updated regex and escape handling for default values in environment variable replacement |
Comments suppressed due to low confidence (1)
src/tests/environment_variable_evaluation/test_environment_variables_braces.py:30
- [nitpick] It may be helpful to add an inline comment clarifying the expected behavior for the nested default scenario to assist future maintainers.
def test_uppercase_with_nested_default_when_unset_in_string_value(self):
| if env_var is None or len(env_var) == 0: | ||
| env_var = default_value | ||
| value = self.update_value_with_resolved_environment(f"\\{matches[0]}", env_var, value) | ||
| escaped = re.sub(r"([*+?{}])", r"\\\1", matches[0]) # escape special characters |
Copilot
AI
Jun 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using re.escape(matches[0]) instead of a custom regex to escape special characters, as it improves readability and reduces the risk of missing additional special characters.
| escaped = re.sub(r"([*+?{}])", r"\\\1", matches[0]) # escape special characters | |
| escaped = re.escape(matches[0]) # escape special characters |
Fix #37