Skip to content

feat(template_utils): add string_list_to_dictionary to template utils#386

Merged
fangliu117 merged 1 commit intodevfrom
template_utils
Aug 4, 2025
Merged

feat(template_utils): add string_list_to_dictionary to template utils#386
fangliu117 merged 1 commit intodevfrom
template_utils

Conversation

@fangliu117
Copy link
Collaborator

@fangliu117 fangliu117 commented Aug 4, 2025

there is indeed a bug in NIDAP code. The intention is clearly to validate that both key and value are non-empty, but the current implementation has two issues:

Logic bug: not value checks if the list is empty, not if the string inside is empty. For "key:", we get value = [""], and not [""] is False, so it doesn't catch the empty value.
Duplicate assignment bug: The line parsed_dict[key] = value[0] appears twice - once inside the else block and again outside, which means duplicate keys would still get added to the dictionary.

Copilot AI review requested due to automatic review settings August 4, 2025 04:09
Copy link
Contributor

Copilot AI left a 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 adds a new utility function string_list_to_dictionary to the template utils module that validates and parses a list of strings in "key:value" format into a dictionary, with customizable error message terminology and duplicate key detection.

  • Adds comprehensive input validation for list format and string entries
  • Implements duplicate key detection with detailed error reporting
  • Provides customizable parameter names for error messages

)
continue

key, *value = map(str.strip, entry.split(":", 1))
Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

The unpacking syntax key, *value creates a list for value even when there's only one element. This causes incorrect validation on line 685 where not value will always be False for non-empty values, and line 696/699 where value[0] is accessed. Should use key, value = map(str.strip, entry.split(":", 1)) instead.

Suggested change
key, *value = map(str.strip, entry.split(":", 1))
key, value = map(str.strip, entry.split(":", 1))

Copilot uses AI. Check for mistakes.
Comment on lines +698 to +700
# Add to dictionary if valid
parsed_dict[key] = value[0]

Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

This code is unreachable because it's placed after a continue statement on line 690. The assignment on line 699 will never execute for the validation path.

Suggested change
# Add to dictionary if valid
parsed_dict[key] = value[0]

Copilot uses AI. Check for mistakes.
Comment on lines +699 to +700
parsed_dict[key] = value[0]

Copy link

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

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

This line duplicates the assignment on line 696 and will overwrite the dictionary entry even for duplicate keys, which contradicts the duplicate detection logic above. This line should be removed since the assignment is already handled on line 696.

Suggested change
parsed_dict[key] = value[0]

Copilot uses AI. Check for mistakes.
@fangliu117
Copy link
Collaborator Author

Note: string_list_to_dictionary has been applied on add_pin_color_rule template successfully as a helper function, for the purpose of migration, here just copy+paste the original NIDAP template code to continue add_pin_color_rule template. We need to do optimize template_utils.string_list_to_dictionary later.

@fangliu117 fangliu117 merged commit d70bb9d into dev Aug 4, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants