Skip to content
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

Merged
merged 2 commits into from
May 31, 2023
Merged

Add support for ini files #25

merged 2 commits into from
May 31, 2023

Conversation

g41797
Copy link
Contributor

@g41797 g41797 commented May 25, 2023

PR for #4
Please review

@g41797 g41797 mentioned this pull request May 25, 2023
@kehoecj kehoecj requested review from jd4235 and jackswiney May 25, 2023 13:38
@kehoecj kehoecj self-assigned this May 25, 2023
	modified:   pkg/filetype/file_type.go
	renamed:    test/fixtures/subdir/bad.ini -> test/fixtures/subdir2/bad.ini
@g41797 g41797 requested a review from jd4235 May 26, 2023 08:55
@g41797 g41797 marked this pull request as ready for review May 26, 2023 08:56
func (iv IniValidator) Validate(b []byte) (bool, error) {
_, err := ini.LoadSources(ini.LoadOptions{}, b)
if err != nil {
return false, err
Copy link
Contributor

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

Copy link
Contributor

@kehoecj kehoecj May 26, 2023

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

Copy link
Contributor Author

@g41797 g41797 May 27, 2023

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.

Two possibilities:

  1. accept limitation and use current implementation
  2. postpone till finding solution

meanwhile pr set to draft mode

Copy link
Contributor

@kehoecj kehoecj May 31, 2023

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.

Copy link
Contributor

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.

@g41797 g41797 marked this pull request as draft May 27, 2023 05:02
@g41797 g41797 requested a review from kehoecj May 27, 2023 05:41
@kehoecj kehoecj marked this pull request as ready for review May 31, 2023 21:47
@kehoecj
Copy link
Contributor

kehoecj commented May 31, 2023

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!

@kehoecj kehoecj merged commit 78a11b1 into Boeing:main May 31, 2023
3 checks passed
Copy link
Contributor

@kehoecj kehoecj left a comment

Choose a reason for hiding this comment

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

LGTM

@kehoecj kehoecj added the OSS Community Contribution Contributions from the OSS Community label Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OSS Community Contribution Contributions from the OSS Community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants