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 options KeyPreserveCase() and KeyNormalizer(func (string) string) #860

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmitry-irtegov
Copy link

Hello!

This is another attempt to implement case-preserving Viper.
Advantages over #635
It uses Option to enable case preserving , so one cannot change this behavior on live Viper instance. As I understand it was main concern about #635
It allows to supply arbitrary func (string) string as key normalizer (though I cannot invent good use cases for this).
It is implemented against current state of master.

Test is taken and adapted from #635

@CLAassistant
Copy link

CLAassistant commented Mar 5, 2020

CLA assistant check
All committers have signed the CLA.

@dmitry-irtegov
Copy link
Author

Hello again!
This PR attempts to fix long-standing issues #131, #260, #293, #371, #373
As I understand, it avoids main concern about PR #635 (in my version, case preservation can be enabled only on Viper creation).

@JounQin
Copy link

JounQin commented Mar 20, 2020

@spf13 Can you please help to review these PRs?

@danmx
Copy link

danmx commented Apr 22, 2020

@sagikazarmark sorry for mentioning you but it seems you're the person with write permissions 😂

@mikaelnousiainen
Copy link

@sagikazarmark Any chance of this PR getting merged? It looks like a very clean -- and backwards-compatible -- approach to the issue with preserving case in keys.

@sagikazarmark
Copy link
Collaborator

Hey folks!

This PR is on my radar (and open in my browser) for a couple months now. I know this has been requested several times, but it's a change that affects Viper at its core and experience shows that the lack of proper test cases makes changes like this dangerous. I can't promise this will land today, in a week or a month, but it will land eventually.

I really appreciate the effort, it's great that we have PRs.

Right now I'm trying to focus on setting a course for Viper v2. We are getting there and I promise this will land eventually, but not today.

Thanks for your patience and the effort!

@sagikazarmark
Copy link
Collaborator

In the meantime, you can help by filling out the feedback form here: https://forms.gle/EUs3EJqMVJSnuKLy5

(FYI: we are going to use that form to measure how important case sensitivity is 😉 )

@oytuntez
Copy link

Well, I just figured about this issue and it's forcing me to rewrite the whole config flow in my application with something else unless I find a work around. This PR doesn't change any default behavior, I think it should be acceptable.

@olegch
Copy link

olegch commented Mar 7, 2023

Hello,
I have just rebased this change to master and updated it for the latest code
Conflicts are resolved and tests pass;
I hope you can merge it.
Thank you for the great project!

@jherman4
Copy link

Hey folks!

This PR is on my radar (and open in my browser) for a couple months now. I know this has been requested several times, but it's a change that affects Viper at its core and experience shows that the lack of proper test cases makes changes like this dangerous. I can't promise this will land today, in a week or a month, but it will land eventually.

I really appreciate the effort, it's great that we have PRs.

Right now I'm trying to focus on setting a course for Viper v2. We are getting there and I promise this will land eventually, but not today.

Thanks for your patience and the effort!

We are now 3.5 years out from this and no merge? I have had a great experience with viper so far but this kind of thing lowers my confidence in the project and I will most likely look at other options in my next project that requires config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants