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 INI support #638

Merged
merged 1 commit into from Dec 6, 2019

Conversation

@techknowlogick
Copy link
Contributor

techknowlogick commented Jan 27, 2019

Fixes #194

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jan 27, 2019

CLA assistant check
All committers have signed the CLA.

@spf13

This comment has been minimized.

Copy link
Owner

spf13 commented Jun 11, 2019

I personally just really dislike the ini format. Is there a reason you need it?

@techknowlogick

This comment has been minimized.

Copy link
Contributor Author

techknowlogick commented Jun 11, 2019

@spf13 I too am not a fan of the format either, which is exactly the reason for this PR. I am a lead on https://github.com/go-gitea/gitea/ which for historical reasons uses ini, however if this PR is accepted, then the project has a roadmap in place to switch to using another configuration format, but in a backwards compatible way.

@sagikazarmark

This comment has been minimized.

Copy link
Collaborator

sagikazarmark commented Jun 11, 2019

Personally I'd rather see a way to extend the current set of parsers (something like drivers in the database/sql package)

@earlzo

This comment has been minimized.

Copy link

earlzo commented Oct 3, 2019

related #194

@earlzo

This comment has been minimized.

Copy link

earlzo commented Oct 13, 2019

hey @spf13, are you gonna to merge this pr?

@techknowlogick

This comment has been minimized.

Copy link
Contributor Author

techknowlogick commented Oct 25, 2019

@sagikazarmark I'd like to see extensions in that way too, perhaps that should is an item for 2.0.

As for now, I've resolved merge conflicts and updated tests so that INI can be understood in 1.x version of viper.

@sagikazarmark

This comment has been minimized.

Copy link
Collaborator

sagikazarmark commented Oct 25, 2019

@techknowlogick TBH I'm not so keen on adding INI support. Actually I would be open to adding it temporarily, because right now there is no way to extend current encoders. But what then? Can we remove it in v2?

Let me think about the extendable encoder solution. If it's feasible to implement in a timely manner, would it be acceptable for you to use that new API to register your custom encoder in your own code?

@techknowlogick

This comment has been minimized.

Copy link
Contributor Author

techknowlogick commented Oct 25, 2019

I would be completely open to that solution, as that is preferable for me (I too dislike INI, but sadly it is a format that I have to work with, and having viper be able to support INI in some fashion would allow me to migrate away from INI).

Also, I think moving between major versions allows for backwards incompatible changes, so if this gets merged now, then INI doesn't have to be in v2 (although hopefully an extendable encoder solution would be available).

@techknowlogick

This comment has been minimized.

Copy link
Contributor Author

techknowlogick commented Nov 30, 2019

@sagikazarmark following up on this, have you discussed with other maintainers re: extending v1 with encoders?

@sagikazarmark

This comment has been minimized.

Copy link
Collaborator

sagikazarmark commented Dec 3, 2019

@techknowlogick no, unfortunately I didn't have the chance. I started playing with an encoding abstraction, but hit the wall with a few problems unfortunately.

I don't want to block this PR, so if you find some time to rebase the branch I'll review and merge it.

@sagikazarmark sagikazarmark added this to the 1.6.0 milestone Dec 3, 2019
@techknowlogick

This comment has been minimized.

Copy link
Contributor Author

techknowlogick commented Dec 3, 2019

@sagikazarmark thanks for the update :) I've just merged the most recent commits to master into this PR. Please let me know if you'd like me to squash this branch.

@sagikazarmark

This comment has been minimized.

Copy link
Collaborator

sagikazarmark commented Dec 3, 2019

@techknowlogick Please do. Also, the branch is still conflicting with master. So I would suggest rebasing instead of merging.

@techknowlogick techknowlogick force-pushed the techknowlogick:add_ini_support branch from 07eb285 to 2a31e57 Dec 3, 2019
pass tests

write out ini file & tests

go fmt

Update viper_test.go
fix test

gofmt
@techknowlogick techknowlogick force-pushed the techknowlogick:add_ini_support branch from 2a31e57 to bb3bcc8 Dec 3, 2019
@techknowlogick

This comment has been minimized.

Copy link
Contributor Author

techknowlogick commented Dec 3, 2019

Squashed & rebased. Looks like Travis is also passing too. 🎉 Please let me know if there is anything else I can do :)

@sagikazarmark sagikazarmark self-requested a review Dec 6, 2019
Copy link
Collaborator

sagikazarmark left a comment

@sagikazarmark sagikazarmark merged commit 351bfe9 into spf13:master Dec 6, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.