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: WriteConfig file extensions check #934

Closed
wants to merge 2 commits into from

Conversation

greg-szabo
Copy link

@greg-szabo greg-szabo commented Jul 5, 2020

Fixes #427 .
Closes #428 .

This PR sets the assumption that if the developer ran SetConfigType during the loading of a config, they should be able to save the config without concern to the file extension.

The current behavior is that the file extension takes precedence during saving, even when SetConfigType was explicitly run.

For example, if the developer loads a .config file by setting the config type to TOML, calling WriteConfig will fail with Unsupported Config Type ".config", even though the developer explicitly told the framework before that it's a TOML file.

The main impact on current use-cases show well in testing: filenames with the wrong extensions are now written in the format defined in SetConfigType, instead of based on the file extension.

@CLAassistant
Copy link

CLAassistant commented Jul 5, 2020

CLA assistant check
All committers have signed the CLA.

@greg-szabo greg-szabo marked this pull request as draft July 5, 2020 02:50
@sagikazarmark
Copy link
Collaborator

@greg-szabo Thanks a lot for the contribution and the well-formulated PR.

As much as I like it though, it's unfortunately a breaking change. The current behavior expects that the file extension takes precedence over the manually configured type (we had a similar situation here: #813 (comment)). Whether that's a good behavior or not is kind of irrelevant from the backward compatibility promise point of view.

As I understand, in the following scenarios this patch would break that promise (correct me if I'm wrong):

  • file: config.json, type: yaml
  • file: .json, type: yaml

I can think of a number of workarounds though to make sure these cases would still work the way they should, and turn to the configured type if all else failed (ie. check if the file extension is supported and use it if it is).

Some tests covering these cases would also be nice.

Let me know if you think I misunderstood something and thanks again for the contribution!

@greg-szabo greg-szabo closed this Sep 6, 2021
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.

WriteConfig should not fail on unknown file extensions when the config type is set
3 participants