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

Updating SafeWriteConfig and SafeWriteConfigAs to match documented be… #763

Merged
merged 2 commits into from Dec 6, 2019

Conversation

javaducky
Copy link
Contributor

…havior.

Methods should throw an error if the config file already exists or if no configpath is configured when not explicitly requesting a write path.

…havior.

 Methods should throw an error if the config file already exists or if no configpath is configured when not explicitly requesting a write path.
@CLAassistant
Copy link

CLAassistant commented Sep 13, 2019

CLA assistant check
All committers have signed the CLA.

@javaducky
Copy link
Contributor Author

I apologize for yet another PR on this issue; I hadn't performed enough due diligence before working up a patch. I'd still like to provide this version as I am providing custom errors to note the specific reason for failure during file initialization.

From my review of open issues, it would appear this may address the following issues: #549, #525, #448 and possibly clear up #433.

Thanks!

Paul

@javaducky
Copy link
Contributor Author

javaducky commented Nov 23, 2019

Hoping to get this PR looked at. As it is now, the current descriptions for the SafeWrite* methods in the README.md do not match reality. This PR will correct that so the expected exceptions are encountered and verifies this as well with the included unit tests.

Here's the description from README which has the intended (and reasonable) usage:

image

If current behavior is preferred, I'd be more than happy to correct the README.md.

@javaducky
Copy link
Contributor Author

@sagikazarmark @spf13

I'm sure this project is encountering the same issue (lack of time, etc.) as Cobra (cobra#959), but I'd be willing to help out. I'm relatively new to the GoLang scene, but have been a software developer longer than I'd like to admit. I appreciate the efforts of all committers and maintainers and would love to pitch in!

@sagikazarmark
Copy link
Collaborator

@javaducky thanks for the offer. It would be a great help if you could review submitted PRs.

The primary reason why the development of Viper stalled is that it became insanely complex, many projects rely on it, so avoiding breaks is not just important, but relatively hard as well.

Another problem is that (obviously) sometimes people want to add features that they want to use, but not necessarily fit into the current Viper ecosystem.

And obviously there is time...

So for now I would say rigorously reviewing PRs is the easiest way to jump on the project.

Copy link
Collaborator

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Thanks @javaducky ! I reviewed your PR. I had a few minor comments, can you please look at those?

viper.go Outdated Show resolved Hide resolved
viper.go Outdated Show resolved Hide resolved
viper_test.go Outdated Show resolved Hide resolved
viper_test.go Outdated Show resolved Hide resolved
viper_test.go Outdated Show resolved Hide resolved
@sagikazarmark sagikazarmark added this to the 1.6.0 milestone Dec 6, 2019
Copy link
Collaborator

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Thanks @javaducky !

@sagikazarmark sagikazarmark merged commit 3a19b6e into spf13:master Dec 6, 2019
@javaducky javaducky deleted the fix-safe-write branch December 6, 2019 13:44
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.

None yet

3 participants