Skip to content

Conversation

kmyk
Copy link
Contributor

@kmyk kmyk commented Feb 22, 2019

solve #113

確認事項は次の2点です。

  • 文言と位置はこれでよさそうか
  • refactoring が必要なところにそれを後回しにして手を加えたが merge して大丈夫なのか

ただし、refactoring を後回しにした点とは、

  • code generator に横断的に変数を足そうとしたらそのようなことができる場所がなかった。なのでいちばんましそうなところにコメント付きで足しました
  • url の文字列は他の module にあるべきだし setup.py と重複があるけど、とりあえず生で書きました
    • atcodertools/release_management/version.py に足すべきだけど、 __url__ を生やすなら他の __author__ とかもすべてそうするべきなことと、setup.py のまわりはミスが怖いので様子見です。少なくともプルリクは分けたい
  • デフォルト生成コードのテストの関係で、 version を上げるときに5箇所の修正が必要になってしまった
    • 私の意見としてはテスト用のリソースをファイルとして持つのが間違い (動的に生成できない + ファイルを作るのはコードを足すのよりはるかに面倒なので、テストが書かれにくくなってしまう) なのですが、これも修正するとすると規模が大きいので様子見です

@codecov-io
Copy link

codecov-io commented Feb 22, 2019

Codecov Report

Merging #117 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
+ Coverage   90.48%   90.49%   +<.01%     
==========================================
  Files          48       48              
  Lines        2165     2167       +2     
==========================================
+ Hits         1959     1961       +2     
  Misses        206      206
Impacted Files Coverage Δ
atcodertools/codegen/template_engine.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3cddc6...495c306. Read the comment docs.

@kyuridenamida
Copy link
Owner

超遅くなりました。最近寝る前に一つひとつ潰そうと頑張っています。

文言と位置はこれでよさそうか

激マイナー指摘ですが、

Generated by {{ atcodertools.version }} {{ atcodertools.url }} (tips: You use the default template now. You can remove this line by using your custom template)

とかどうでしょう。

refactoring が必要なところにそれを後回しにして手を加えたが merge して大丈夫なのか

うーん、バージョンアップデートする度にテストの文言を修正しないといけないというのは悩ましいですね。

私の意見としてはテスト用のリソースをファイルとして持つのが間違い (動的に生成できない + ファイルを作るのはコードを足すのよりはるかに面倒なので、テストが書かれにくくなってしまう) なのですが、これも修正するとすると規模が大きいので様子見です

ファイルを追加するというのが手間というのは意外な感想ですが、IDE使ってるからかもしれません。
(個人的には無理やりテキストファイルをコードに埋め込むとかのほうが良くないと思っています)

今回みたいな静的ファイルで管理してると更新が必要みたいなケースは、そもそもグローバル変数に依存してコードを生成することに起因してると思っています。色々選択肢はありますが、現実的かなと思う選択肢は3つです。

  1. テスト側からバージョン情報を挿入できるようにする(グローバル変数を消す)
  2. デフォルトテンプレートに関しては、テンプレートで生成されたコードの一致性を確かめず、標準出力を見るに徹する
  3. 1行目を無視する

行儀の良さだけを考えれば1なんですが、それをして嬉しいメリットが正直テストしかない + こんなことで困る例はかなり特殊なので、(2)でも(3)でも構いません。意外と(3)が労力に対してインパクトが大きいんじゃないでしょうか。これでメンテに苦しむ未来はこのケースに関しては思いつきません。

urlに関してはまた考えましょう。今はこれでいいです。#109で解決されるべきという点について完全に同意します。

@kmyk
Copy link
Contributor Author

kmyk commented Mar 4, 2019

とりあえず文言を修正してコミットしました。提案されたものの方が自然そうに見えたのでそのまま使いました。

解決策の選択肢の3つは正しそうで他には思い付きません。
ただし (3) への評価はおおよそ正しそうですが、このコメントが必ずしも1行目にあるとは限らない点を忘れていそうなのは補足しておきたいです。一番上は嫌だなあと思ったので被生成部分の上に置いてしまっていますし、特にLL系だとshebangがあるので回避も不可能です。
それでも (3) が優位そうなので (3) をあとで実装しておきます。先にマージするかしないかはまかせます。


ところでファイルの追加が手間でないというのはこちらにとってもすこし意外です。
言うとおりIDEとeditorの差と考えればそうなる気がします。特にディレクトリの登り降りを含めるとIDEが圧倒的に強いです。
テキストファイルのコードへの埋め込みについても同様で、これらはJavaの文化とUnixの文化の対立に見えます。私にとっては、ファイルであるような定数とファイルでないような定数を区別する方が不自然に見えますが、その背景にはhere-documentがありそうです。

@kyuridenamida
Copy link
Owner

(3)ですが、なるほど、そういう経緯が・・・
どうせ汚いことをやっていることは誰が見ても自明にわかるので、もうアドホックにバージョンっぽい文字列があったら適当な文字列に置換して比較するとかでいいんじゃないですかね。テストだし。

@kyuridenamida
Copy link
Owner

とりあえず作業中かもしれませんがこの機能はほしいので先にマージしときます。

@kyuridenamida kyuridenamida merged commit d694ff1 into kyuridenamida:master Mar 5, 2019
@kyuridenamida kyuridenamida modified the milestones: 1.1.0, 1.1.3 Mar 5, 2019
@kyuridenamida kyuridenamida changed the title Add PR to the default templates (#113) Add PR to the default templates Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants