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 to save file with no extension #813

Merged
merged 4 commits into from Feb 19, 2020

Conversation

gssbzn
Copy link
Contributor

@gssbzn gssbzn commented Dec 15, 2019

The suppport introduced for files with no file extension is only partial as trying to save the config file would fail with <file name> equires valid extensio
This adds support to saving such files

The suppport introduced for files with no file extension is only partial as trying to save the config file would fail with `<file name> equires valid extensio`
This adds support to saving such files
@claassistantio
Copy link

claassistantio commented Dec 15, 2019

CLA assistant check
All committers have signed the CLA.

@gssbzn
Copy link
Contributor Author

gssbzn commented Jan 2, 2020

@sagikazarmark any chance to give this a look? it would be great to fully support config files with no extensions

err := v.ReadConfig(bytes.NewBuffer(yamlExample))
if err != nil {
t.Fatal(err)
testCases := map[string]struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice refactor!

Currently, the config type is always inferred from the file name, not the original config type. So if the config type was yaml, but I provided a json file name, the output content was in JSON.

Can you please add a test case that verifies this is still the case?

Because (unfortunately) as far as I can tell, it is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just making sure I got the right idea here, are you saying to default on writeConfig to the extension file and if this is blank then use the setting for config type?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup. At least, that's compatible with the current behaviour, taking the original configType by default is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha 👍

@gssbzn
Copy link
Contributor Author

gssbzn commented Jan 10, 2020

@sagikazarmark I added the requested changes, this complicated the test a bit as the read depends always of the configType and I don't know if we should keep the error of undefined type or just relay on the extensions check

@gssbzn
Copy link
Contributor Author

gssbzn commented Jan 22, 2020

@sagikazarmark any update on this PR?

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.

@gssbzn Sorry for the delay, thanks again for the PR and the nice refactor!

@sagikazarmark sagikazarmark merged commit 97ee7ad into spf13:master Feb 19, 2020
@sagikazarmark sagikazarmark linked an issue Feb 19, 2020 that may be closed by this pull request
@sagikazarmark sagikazarmark added this to the 1.7.0 milestone Apr 9, 2020
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.

Cannot Write Config With No Extension
3 participants