-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add support for ini files #25
Conversation
modified: pkg/filetype/file_type.go renamed: test/fixtures/subdir/bad.ini -> test/fixtures/subdir2/bad.ini
func (iv IniValidator) Validate(b []byte) (bool, error) { | ||
_, err := ini.LoadSources(ini.LoadOptions{}, b) | ||
if err != nil { | ||
return false, err |
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.
The error output needs to include the line and column number. For example:
Error at line 1 column 3:
Since ini does not seem to output that by default you'll need to calculate it. Look at how that's being done in the json validator
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.
Created #26 to help enforce this in the future
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 didn't find possibility to calculate line&column
Checked another go ini solutions e.g.
- ini/parser
- github.com/vaughan0/go-ini - very old but has ErrSyntax
Two possibilities:
- accept limitation and use current implementation
- postpone till finding solution
meanwhile pr set to draft mode
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 looked into it as well and you're right - there is no way to get the line/column number from the error output.
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.
Wrote an enhancement request to go-init to add this output to the error message.
We're going to accept this MR as-is (without the line/column number in the error output) to be able to offer ini validation and work line/column output in a separate issue. Thanks @g41797 for the the contribution! |
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.
LGTM
* Add support for ini files * Fixes after review modified: pkg/filetype/file_type.go renamed: test/fixtures/subdir/bad.ini -> test/fixtures/subdir2/bad.ini
* Add support for ini files * Fixes after review modified: pkg/filetype/file_type.go renamed: test/fixtures/subdir/bad.ini -> test/fixtures/subdir2/bad.ini
PR for #4
Please review