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

[breaking change] normalize localRoot to absolute path #87

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Conversation

Songmu
Copy link
Member

@Songmu Songmu commented Oct 9, 2023

blogConfig.LocalPathを設定読み込み時に絶対パスに正規化して統一するようにしたい。また、設定ファイルに相対パスが指定されていた場合に、設定ファイルからの相対位置に解決することとしたい。これは統一感や将来の機能拡張のため。具体的には例えば blogsync push でファイルの配置位置がどのブログのエントリーか判別するためなどにそのようにしたい。

設定ファイルの位置からの相対位置で絶対パスを解決するようにすると、グローバル設定 (.config/blogsync/config.yaml) に相対パスが設定されていた場合に非互換変更になる。現状はblogsync実行位置からの相対位置でLocalRootが解決されるため。ローカル設定 (blogsync.yaml) の場合は、blogsync実行ディレクトリとファイル配置ディレクトリが同一なので非互換変更にはならない。

ただ、一般的には設定ファイルからの相対位置で相対パスが解決されることが期待されると思うし、実際blogsyncの実行パスによって、エントリーがpullされてくる位置がコロコロ変わるのもおかしな話だと思う。なので、できれば挙動を変えたい。

以下の選択肢がある。

  1. 現状の挙動(blogsync実行位置からの相対位置)を維持する
  2. globalConfigの場合のみ実行位置からの相対位置とする
    • これは実は1と挙動は同じ。将来的に設定ファイルをコマンドラインオプションで指定できるようにした場合などに変わりうる
    • これは挙動として複雑で紛らわしいので筋が悪い
  3. localConfigの場合のみ相対パス設定を許容する
    • globalConfigに相対パスが設定されていたらエラーとする
    • .config/blogsync/config.yaml からの相対パスにファイルが書き込まれるの嫌な感じがするのでこれはfool proof的にはやったほうが良いかも
      • 現在グローバル設定に相対パスを指定しているユーザーの挙動が変わることになるので、それまでと違う想定外の位置にpullされるよりかはエラーメッセージを出して知らせたほうが親切という話もある
  4. 何も考えず、相対パスを設定ファイルからの相対位置に解決する

今の実装は4。3にはしても良いかも知れない。

→ 3にするのそこをケアするためにコードを複雑にしても仕方ないので一旦実装なくていいかと思った。

@Songmu
Copy link
Member Author

Songmu commented Oct 9, 2023

https://github.com/hatena/Hatena-Blog-Workflows-Boilerplate
これでは、ローカル設定( blogsync.yaml ) を使う想定になっているので、特に挙動を壊すことはなさそうか。

config_test.go Outdated Show resolved Hide resolved
config_test.go Outdated Show resolved Hide resolved
config_test.go Outdated Show resolved Hide resolved
config_test.go Outdated Show resolved Hide resolved
@coveralls
Copy link

Pull Request Test Coverage Report for Build 6459116609

  • 20 of 25 (80.0%) changed or added relevant lines in 2 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.2%) to 27.925%

Changes Missing Coverage Covered Lines Changed/Added Lines %
config.go 19 24 79.17%
Files with Coverage Reduction New Missed Lines %
main.go 2 19.05%
config.go 3 84.27%
Totals Coverage Status
Change from base Build 6453804876: 0.2%
Covered Lines: 179
Relevant Lines: 641

💛 - Coveralls

@Songmu Songmu changed the title normalize localRoot to absolute path [breaking change] normalize localRoot to absolute path Oct 10, 2023
@Songmu Songmu marked this pull request as ready for review October 13, 2023 15:09
@Songmu Songmu merged commit d4dae75 into master Oct 13, 2023
7 checks passed
@Songmu Songmu deleted the fix-config branch October 13, 2023 15:11
@Songmu Songmu added the minor label Oct 13, 2023
@github-actions github-actions bot mentioned this pull request Oct 13, 2023
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.

None yet

2 participants