-
Notifications
You must be signed in to change notification settings - Fork 32
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
accept UTF8-BOM when reading dashboard or monitor from JSON file #622
Conversation
fallback := unicode.UTF8.NewDecoder() | ||
r := transform.NewReader(src, unicode.BOMOverride(fallback)) |
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.
I do like it a lot!
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.
Could you add a test?
I added similar BOM handling to monitors code. (it will be called by |
tmpFile, err := os.CreateTemp("", "") | ||
if err != nil { | ||
t.Errorf("should not raise error: %v", err) | ||
} | ||
defer os.Remove(tmpFile.Name()) |
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.
testing.T.TempDir is useful.
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.
I had tried it on 8df6538 , 3295eed , and db949f8 but all of them went failed on Windows (I saw golang/go#50510 )
It would be better to use it but I keep CreateTemp code at this time for Windows.
Passing a JSON file with a UTF-8 BOM to
mkr dashboards push --file-path
causes an error that is difficult for users to understand.I understand that "JSON files must not contain a BOM", but I thought it would be more user-friendly to ignore the BOM part and read the file without making an error, so I made a patch.
Test
t-nobom.json
t-withbom.json