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

fix issue 427: use v.getConfigType to determine config type in WriteConfig #428

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

fix issue 427: use v.getConfigType to determine config type in WriteConfig #428

wants to merge 1 commit into from

Conversation

mildwonkey
Copy link

use the existing getConfigType function instead of figuring it out anew
#427

@CLAassistant
Copy link

CLAassistant commented Dec 20, 2017

CLA assistant check
All committers have signed the CLA.

@mildwonkey
Copy link
Author

@spf13 @bep sorry to do this, but - ping! review pretty please

@sgettys
Copy link

sgettys commented Nov 27, 2019

Is this ever going to make it into master?

@ghost
Copy link

ghost commented Mar 6, 2020

I'd like to know when this gets merged as well

@Fuhrmann
Copy link

Could this be merged?

@greg-szabo
Copy link

greg-szabo commented Jul 7, 2020

I think using getConfigType is not enough. You can build a configuration without defining a filename and then you can try to run SafeWriteConfigAs which again doesn't require to have a filename in your configuration (you are giving the filename as an input parameter to the function).

The problem is that getConfigType will check the filename to get the extension from it. So, in the above-mentioned case, in SafeWriteConfigAs the result of getConfigType will be an empty string: no filename was set in the object, there's no way to figure out it's file extension.

I've tried to figure out why writeConfig is dealing with file extensions and I think the reason is that there's the point where all filenames are in place and the file extension can be figured out. I think the real issue is that the file extension becomes an ultimate priority, even in cases where the developer explicitly set a specific config type using SetConfigType.

In #934 I'm proposing a solution that's making the config type the priority during write, so files with weird extensions (but a properly set ConfigType) can also be saved.

Edit: Also #934 is up-to-date with master. ;) I sure hope that helps with the merge time.

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

5 participants