Skip to content

fix(history): use dirname() for correct config path calculation#22

Merged
cunoe merged 1 commit intomainfrom
fix/issue-8-history-path
Mar 31, 2026
Merged

fix(history): use dirname() for correct config path calculation#22
cunoe merged 1 commit intomainfrom
fix/issue-8-history-path

Conversation

@cunoe
Copy link
Copy Markdown
Contributor

@cunoe cunoe commented Mar 31, 2026

Closes #8

Summary

  • Replace incorrect join(path, '..') with dirname() for calculating config directory
  • Ensures database path .envx/envx.db is found in the correct location

Copy link
Copy Markdown

@leaperone-bot leaperone-bot left a comment

Choose a reason for hiding this comment

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

🤖 Code Review 🟢

代码审查:使用 dirname() 替代 join(path, '..') 计算配置目录

变更概述

history.ts 中的 join(process.cwd(), options.config, '..') 替换为 dirname(configPath),与 load.ts 中已有的模式保持一致。

分析

正确性

  • 虽然在大多数实际场景中 join(path, '..')dirname(path) 的结果是等价的(经过实际验证),但 dirname() 是获取父目录的标准、语义化做法。join(path, '..') 依赖路径拼接再归一化来间接获取父目录,意图不如 dirname() 清晰。
  • 修复后的代码还复用了已计算好的 configPathdirname(configPath)),而不是重复拼接 process.cwd()options.config,消除了冗余,减少了两处表达式不一致的风险。

一致性

  • load.ts 已使用 dirname() 模式,此修改使 history.ts 与之对齐,保持了代码库的统一风格。

旧代码的潜在问题

  • 旧代码中 configDir 的计算独立于 configPath,如果未来有人修改了其中一处的默认值逻辑而忘记另一处,就会产生不一致的 bug。新代码通过 dirname(configPath) 直接复用 configPath,从根本上消除了这种风险。

小建议(非阻塞)

  1. options.config 的默认值冗余configPath 中的 options.config || './envx.config.yaml' 实际上不需要 fallback,因为 Commander 的 .option() 已经定义了默认值 './envx.config.yaml'。可以考虑简化为 join(process.cwd(), options.config!)。这不在本 PR 范围内,仅供参考。

  2. 缺少测试覆盖:项目目前没有测试文件。建议后续为 history 命令添加单元测试,特别是路径计算逻辑。

结论

改动最小、意图明确,提升了代码的可读性和可维护性。可以合并。

@cunoe cunoe merged commit 4f697d5 into main Mar 31, 2026
1 of 2 checks passed
@cunoe cunoe deleted the fix/issue-8-history-path branch March 31, 2026 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

history 命令配置路径计算错误

2 participants