Skip to content

refactor: Address Copilot code review suggestions #21

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.

Sign up for GitHub

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

Merged
0xvasanth merged 1 commit into from
Nov 1, 2025
Merged

refactor: Address Copilot code review suggestions #21

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions src/environment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,18 @@ impl Environment {
}
}

/// Normalize a key for storage in the flat map based on nested mode setting.
///
/// In nested mode, preserves the original case for proper splitting.
/// In flat mode, converts to lowercase for backward compatibility.
fn normalize_key(&self, key: &str) -> String {
if self.nested {
key.to_string()
} else {
key.to_lowercase()
}
}
Comment on lines +246 to +252
Copy link

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

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

The normalize_key method preserves case in nested mode, but lines 424 and 428-429 unconditionally lowercase keys even in nested mode. This creates an inconsistency: keys stored in flat_map via normalize_key (lines 380, 405) preserve case in nested mode, but when split and inserted into result (lines 424, 428-429), they are lowercased. This breaks the intended behavior documented in the method's comment. Either remove the lowercasing at lines 424 and 428-429 when in nested mode, or update normalize_key to always lowercase.

Copilot uses AI. Check for mistakes.

fn parse_env_value(value: &str) -> Value {
if let Ok(b) = value.parse::<bool>() {
return json!(b);
Expand Down Expand Up @@ -272,19 +284,19 @@ impl Environment {
/// This helper function takes a flat key path (e.g., ["http", "server", "port"])
/// and creates the necessary nested structure in the map, inserting the value
/// at the deepest level.
fn insert_nested(map: &mut Map<String, Value>, parts: &[&str], value: Value) {
fn insert_nested(map: &mut Map<String, Value>, parts: &[String], value: Value) {
if parts.is_empty() {
return;
}

if parts.len() == 1 {
// Base case: insert the value at this key
map.insert(parts[0].to_string(), value);
map.insert(parts[0].clone(), value);
return;
}

// Recursive case: get or create the nested object
let key = parts[0].to_string();
let key = parts[0].clone();
match map.entry(key) {
serde_json::map::Entry::Occupied(mut occ) => {
if let Value::Object(ref mut nested) = occ.get_mut() {
Expand Down Expand Up @@ -365,12 +377,7 @@ impl Environment {

if key_check.starts_with(&prefix_str) {
let trimmed = key_check[prefix_str.len()..].trim_start_matches(&self.separator);
// Keep case for nested mode, lowercase for flat mode
let key_for_map = if self.nested {
trimmed.to_string()
} else {
trimmed.to_lowercase()
};
let key_for_map = self.normalize_key(trimmed);
Comment on lines 379 to +380
Copy link

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

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

When case_sensitive is false, key_check is uppercased (line 375), so trimmed contains an uppercased string. Passing this to normalize_key in nested mode preserves the uppercase version, which likely breaks case-sensitive comparisons later. The original key should be used instead of key_check when extracting trimmed.

Copilot uses AI. Check for mistakes.
flat_map.insert(key_for_map, Self::parse_env_value(&value));
}
} else {
Expand All @@ -395,12 +402,7 @@ impl Environment {

if key_check.starts_with(&prefix_str) {
let trimmed = key_check[prefix_str.len()..].trim_start_matches(&self.separator);
Copy link

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

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

When case_sensitive is false, key_check is uppercased (line 400), so trimmed contains an uppercased string. Passing this to normalize_key in nested mode preserves the uppercase version, which likely breaks case-sensitive comparisons later. The original override_key should be used instead of key_check when extracting trimmed.

Suggested change
let trimmed = key_check[prefix_str.len()..].trim_start_matches(&self.separator);
let trimmed = override_key[prefix_str.len()..].trim_start_matches(&self.separator);

Copilot uses AI. Check for mistakes.
// Keep case for nested mode, lowercase for flat mode
let key_for_map = if self.nested {
trimmed.to_string()
} else {
trimmed.to_lowercase()
};
let key_for_map = self.normalize_key(trimmed);
flat_map.insert(key_for_map, Self::parse_env_value(override_value));
}
} else {
Expand All @@ -425,9 +427,7 @@ impl Environment {
// Lowercase each part individually
let lowercase_parts: Vec<String> =
parts.iter().map(|p| p.to_lowercase()).collect();
let lowercase_parts_refs: Vec<&str> =
lowercase_parts.iter().map(|s| s.as_str()).collect();
Self::insert_nested(&mut result, &lowercase_parts_refs, value);
Self::insert_nested(&mut result, &lowercase_parts, value);
}
} else {
// Keep keys flat (backward compatible behavior)
Expand Down
Loading