feat: implement configuration parsing and v1-to-v2 migration logic for mcp-toolbox#3034
feat: implement configuration parsing and v1-to-v2 migration logic for mcp-toolbox#3034skbss wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds logic to the configuration processor to normalize 'description' fields by replacing newlines with spaces and trimming whitespace. It also includes a new 'issues.json' file. Reviewers identified that the string normalization could be improved by using 'strings.Fields' and 'strings.Join' to better handle Windows-style line endings and multiple consecutive spaces. Furthermore, the 'issues.json' file should be removed as it appears to be an accidental inclusion of metadata.
| if s, ok := item.Value.(string); ok { | ||
| item.Value = strings.TrimSpace(strings.ReplaceAll(s, "\n", " ")) | ||
| } |
There was a problem hiding this comment.
The current implementation only handles Unix-style newlines (\n). If the input configuration uses Windows-style line endings (\r\n), the carriage return character (\r) will remain in the description string. Additionally, replacing newlines with spaces can result in multiple consecutive spaces if the original text had indentation or trailing spaces. Using strings.Fields combined with strings.Join is a more robust way to normalize the description into a single line with single spaces, as it automatically handles all types of whitespace and line endings.
| if s, ok := item.Value.(string); ok { | |
| item.Value = strings.TrimSpace(strings.ReplaceAll(s, "\n", " ")) | |
| } | |
| if s, ok := item.Value.(string); ok { | |
| item.Value = strings.Join(strings.Fields(s), " ") | |
| } |
| { | ||
| "number": 3023, | ||
| "title": "migration add \\n instead of separating lines for description" | ||
| } |
There was a problem hiding this comment.
This file appears to be an accidental inclusion in the repository. It contains issue metadata that should typically be part of the pull request description or tracked in an external system, rather than being committed as a source file. Please remove it if it is not required for the application's functionality.
|
Hi @skbss! Thanks so much for putting this together. I actually just investigated this and submitted a fix here: #3050. It turns out the root cause was that list inside the string literal block. Let me know if you found other issue with the migration process. I’m sorry I missed your PR before jumping in! To make sure we don't accidentally double up on work in the future, would you mind dropping a quick comment on the issue when you start? If you link the issue in your PR description too, it helps us track everything perfectly. Really appreciate the effort you put into this! Will close this PR for now, thank you again!! |
Description
PR Checklist
CONTRIBUTING.md
bug/issue
before writing your code! That way we can discuss the change, evaluate
designs, and agree on the general idea
!if this involve a breaking change🛠️ Fixes #<issue_number_goes_here>