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

Adding SaveConfig(). #153

Closed
wants to merge 4 commits into from
Closed

Adding SaveConfig(). #153

wants to merge 4 commits into from

Conversation

g3rk6
Copy link

@g3rk6 g3rk6 commented Jan 11, 2016

It would be nice to have a way to store configuration into a file. There could be better way of doing it, but this is the version that works for me.

@CLAassistant
Copy link

CLAassistant commented Jan 11, 2016

CLA assistant check
All committers have signed the CLA.

}

f, err := os.Create(v.getConfigFile())
defer f.Close()

Choose a reason for hiding this comment

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

defer should after check error?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed it. Thanks for pointing it out!

…r checking must be first otherwise it will cause panic.
@doublerebel
Copy link

LGTM 👍 . Just needs docs.

EDIT: Would also close #33 .

@spf13
Copy link
Owner

spf13 commented Feb 8, 2016

Thanks for the contribution!
I love this idea, and the approach, but I think the PR needs some tweaking.

A few suggestions:

  • Remove the debug line you left in
  • Create a function in util.go that is called marshallConfigWriter
    It would do the opposite of the unmarshallConfigReader function presently there
  • Expand your current code to handle the additional types (hcl & prop)
  • rename SaveConfig to WriteConfig()
  • perhaps add another function called SafeWriteConfig() which only writes when the file doesn't currently exist.
  • Add some tests. Afero would be very helpful here.
  • Add some docs. It's a great feature, people should know about it.

I think you've done most of the work already, it's just mostly a question of reorganizing things.

Let me know if you want help or have questions. Happy to help.

@bryanl
Copy link
Contributor

bryanl commented Jul 22, 2016

This idea is six months old, so I'd assume this PR is dead?

@AntonioMeireles
Copy link

@g3rk6 hi, anything (other than time) blocking you from progressing in this front ?

@g3rk6
Copy link
Author

g3rk6 commented Aug 2, 2016

Sorry for the long delay. At the moment the only issue is time and currently super busy. I'll try to make some progress as soon as I get some time. Thanks.

@theherk
Copy link
Contributor

theherk commented Dec 5, 2016

@g3rk6 I'd love to see this fully implemented. I think it is looking good. A group I am part of may have some time to dedicate to helping with it here shortly.

This was referenced Dec 9, 2016
@spf13
Copy link
Owner

spf13 commented Apr 21, 2017

this is stale and it appears #287 has carried it forward. Closing this.

@spf13 spf13 closed this Apr 21, 2017
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.

8 participants