Skip to content

Conversation

@kqito
Copy link
Contributor

@kqito kqito commented Sep 25, 2025

Summary

#76

Added support for .yaml extension in addition to the existing .yml support. The tool now accepts both formats, with .yaml taking precedence when both exist.

Changes

  • Support both .yaml and .yml configuration files
  • Validate file extensions and provide clear error messages for unsupported formats

@kqito kqito self-assigned this Sep 25, 2025
return Err(ConfigError::Message(format!(
"Unsupported configuration file format '.{}'. Only {} files are supported.",
extension,
CONFIG_FILE_EXTENSIONS.join(" and ")
Copy link
Contributor

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 🤔

Copy link
Contributor Author

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

@kqito kqito force-pushed the feat/add-support-for-yaml branch from f1e87f1 to 1e4e1ff Compare September 25, 2025 07:04
Copy link
Contributor

@arabian9ts arabian9ts left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@dora1998 dora1998 left a 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!

Comment on lines 113 to 116
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));
Copy link
Contributor

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.

Copy link
Contributor Author

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

e730803

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
Copy link
Contributor

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. 💭

Copy link
Contributor Author

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?

Copy link
Contributor

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!

@kqito kqito merged commit f5f175c into main Sep 25, 2025
10 checks passed
@kqito kqito deleted the feat/add-support-for-yaml branch September 25, 2025 09:32
@github-actions github-actions bot mentioned this pull request Sep 25, 2025
@kqito kqito mentioned this pull request Sep 25, 2025
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.

4 participants