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

Support config files with no extensions #722

Merged

Conversation

pedromss
Copy link
Contributor

@pedromss pedromss commented Jun 20, 2019

Support config files with no extension.

Closes: #390 This was a mistake
Closes: #336

Similar to https://github.com/spf13/viper/pull/357/files which appears to have been forgotten.

Difference is: attempt a no extension right away instead of messing with the extension append and iterating through the extensions.

@CLAassistant
Copy link

CLAassistant commented Jun 20, 2019

CLA assistant check
All committers have signed the CLA.

@gjrtimmer
Copy link

Hi, I really want this, any news when it will be merged ?

@pedromss maybe a update of the README is required ?
Maybe some note that if using a config file without extension viper.SetConfigType(...) is required for viper to deserialize the config ?

@pedromss
Copy link
Contributor Author

pedromss commented Sep 9, 2019

@gjrtimmer I don't know what else is required to get this approved. Seems like this repo isn't maintained judging by the state of the PR and issue section

¯_(ツ)_/¯

@dmacvicar
Copy link

dmacvicar commented Oct 17, 2019

I like this approach as well.
Can we please merge either this or my own #357, which is open for more than 2 years?

@sagikazarmark
Copy link
Collaborator

@pedromss Just for me to understand: you want to explicitly set a config name with an extension?

viper.SetConfigNam("config.yaml")

And you want to search for that specific file within the configured search paths?

Is that correct?

@pedromss
Copy link
Contributor Author

pedromss commented Nov 2, 2019

@sagikazarmark no. The idea is to be able to have configuration files that don't have an extension in their name. As is many times the case with tools which have config files in the home folder for example.

So supporting .toolconf instead of .toolconf.yaml is the goal of this PR

@sagikazarmark sagikazarmark added this to the 1.6.0 milestone Nov 2, 2019
@sagikazarmark
Copy link
Collaborator

@pedromss I see. Thanks for the explanation.

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.

Can you please add a few lines and an example about this feature to the readme? Maybe after the Reading Config Files section.

Apart from that ✔️

@pedromss
Copy link
Contributor Author

pedromss commented Nov 5, 2019

@sagikazarmark please take another look now

@sagikazarmark
Copy link
Collaborator

Thanks @pedromss !

@sagikazarmark
Copy link
Collaborator

@pedromss actually, I wonder if the condition should be moved after the the loop, to make sure existing behavior isn't broken (eg. there is a config and a config.yaml file in a folder. this PR would break existing code in those cases).

WDYT?

@pedromss
Copy link
Contributor Author

pedromss commented Nov 6, 2019

@sagikazarmark I had thought to resolve it immediately without "unnecessarily" walking the files in the folder. But what you say makes more sense.

Changed it.

@sagikazarmark
Copy link
Collaborator

Awesome, thanks!

This is a one time step, so I wouldn't worry about unnecessary checks for non-existing files.

@andig
Copy link
Contributor

andig commented Jan 10, 2020

I'm a bit confused as to how far this fixes #390. #390 doesn't seem to be about extension-less files?

@pedromss
Copy link
Contributor Author

You're correct 😕 I must have made a mistake when analysing the issue. Reading it again, I would say this comment #390 (comment) made me think the problem was the no extension, which was a problem in it self but not of that issue.

Thank you. I'll edit this description and reopen #390

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.

config file not found viper.ConfigFileNotFoundError when file most definitely exists.
6 participants