-
Notifications
You must be signed in to change notification settings - Fork 1
Add support for .yaml extension alongside .yml #77
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
Conversation
core/src/config/builder.rs
Outdated
| return Err(ConfigError::Message(format!( | ||
| "Unsupported configuration file format '.{}'. Only {} files are supported.", | ||
| extension, | ||
| CONFIG_FILE_EXTENSIONS.join(" and ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to change and to or in this line 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! Changed and to or as recommended.
1e4e1ff
f1e87f1 to
1e4e1ff
Compare
arabian9ts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
dora1998
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I posted a few comments, but it's up to you. Thanks!
core/src/config/builder.rs
Outdated
| let config_file_path = match self.config_file_path { | ||
| Some(path) => { | ||
| let has_valid_extension = | ||
| CONFIG_FILE_EXTENSIONS.iter().any(|ext| path.ends_with(ext)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo. When specifying the -c option, I believe there's no need to check the file extension. This is sufficient since exceptions during deserialization are already handled in the subsequent load_config process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok , removed this validation for explicit -c paths and let load_config handle any format issues once
cli/src/subcommand/init.rs
Outdated
| fn init_prompt() -> Result<InitPromptResult, Error> { | ||
| // TODO: Add support for finding root of the project | ||
| let is_config_file_exists = std::path::Path::new(DEFAULT_CONFIG_FILE_PATH).exists(); | ||
| let existing_configs: Vec<&str> = DEFAULT_CONFIG_FILE_PATHS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo. If the ConfigBuilder can check for the existence of the config file, we might not have to worry about variations in the YAML file extension when implementing the init command. 💭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on the shared logic. changed to move it to ConfigBuilder by e1d3198 (is this what you were suggesting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I expected. Thanks!
Summary
#76
Added support for
.yamlextension in addition to the existing.ymlsupport. The tool now accepts both formats, with.yamltaking precedence when both exist.Changes
.yamland.ymlconfiguration files