Skip to content

Junoser-Squashの実装 #10

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

Merged
merged 1 commit into from
Jan 24, 2018
Merged

Conversation

shogosanz
Copy link
Contributor

@shogosanz shogosanz commented Jan 21, 2018

Junoser-Squashの実装

ref #8

わかりやすくするために、余計なものを取っ払ってsquashの機能のみに絞ったPRにしております。
頂いたgistに、あるset文の部分になるようなset文を除去するremove_subcommand関数を追加しました。

実行

(16:40:33): junoser$cat test_config
set interfaces em0 unit 0 family inet mtu 1500
set interfaces em0 unit 0 family inet6
set interfaces em1 unit 1
set interfaces em2
set interfaces em100 unit 0 family inet mtu 1500
set interfaces em100 unit 0 family inet6

delete interfaces em0 unit 0 family inet6
delete interfaces em100 unit 0

(16:39:31): junoser$ruby exe/junoser-squash test_config
set interfaces em0 unit 0 family inet mtu 1500
set interfaces em1 unit 1
set interfaces em2
set interfaces em100

名前に関して追記

今更名前についてまた議論するのはどうかとも思いましたが、単純に使っていて抱いた感想を言います。
squashはどうしても複数のものを一つにするという風に見える都合上、2つのファイルを統合するようなイメージを持ちました。
そこで、junoser-compressはどうでしょうか。
compressならば、内容の圧縮→つまりdeleteやinsertなどの文も全部圧縮した損失ない最終型へ
というイメージがつきます。

@codeout
Copy link
Owner

codeout commented Jan 22, 2018

ありがとうございます、こちら大筋よさそうと思いました。

次の点なんとかなればマージしようと思います。

  • 破壊的な操作と、非破壊操作を区別したい

    • 既存の名前空間に混ぜず、exe/junoser-squashlib/junoser/squash.rb だけでやる
    • junoser はparser なので、非破壊を意図しています。たとえば
      set interfaces xe-0/0/0
      set interfaces xe-0/0/0 unit 0 address 10.0.0.1/30
      
      は冗長ですがvalid です。意図を持って冗長にする場合も考えられるので、valid であるかぎり入力を尊重したい = 我々は入力の意図を汲めないので触らない方が良い、と思っています。
  • テストの追加

    • remove_subcommandO(n^2) になっているの、工夫の余地ありなのですが…リファクタリングの前にテストあるとうれしいですね

@codeout
Copy link
Owner

codeout commented Jan 22, 2018

名前に関して追記

名前については、 compress よりはsquash のほうが良いと思いました。

compressならば、内容の圧縮→つまりdeleteやinsertなどの文も全部圧縮した損失ない最終型

はおっしゃる通りのイメージなのですが、「入力に対する破壊的操作ですよ」という点明示したいのです。

(とはいうもののsquash もすごく気に入ってるわけではないので…マージまでに思いついたら引き続き名前募集です)

@shogosanz
Copy link
Contributor Author

破壊的な操作と、非破壊操作を区別したい
junoser はparser なので、非破壊を意図しています。
既存の名前空間に混ぜず、exe/junoser-squash とlib/junoser/squash.rb だけでやる

なるほど。
これについて、分離致しました。

は冗長ですがvalid です。意図を持って冗長にする場合も考えられるので、valid であるかぎり入力を尊重したい = 我々は入力の意図を汲めないので触らない方が良い、と思っています。

ref #8 (comment)
のコメントも含めて考慮し、

新しいコミットでは、

set interfaces xe-0/0/0
set interfaces xe-0/0/0 unit 0 address 10.0.0.1/30

の場合は残し、

set interfaces xe-0/0/0 unit 0 address 10.0.0.1/30
set interfaces xe-0/0/0

の場合は削除する実装になっております。

名前については、 compress よりはsquash のほうが良いと思いました。
「入力に対する破壊的操作ですよ」という点明示したいのです。

なるほど。そういう意図があったのですね。理解しました。

@codeout
Copy link
Owner

codeout commented Jan 23, 2018

ありがとうございます!

#10 (comment)

で指摘さしあげたもののうち、一部commit いただきました。

次の点なんとかなればマージしようと思います。

  • 破壊的な操作と、非破壊操作を区別したい
    • exe/junoser-squash
    • lib/junoser/squash.rb だけでやる
      • squash はdisplay でないので、モジュール名前空間の微調整をおねがいします
  • テストの追加
    • ご検討いただけるとうれしいです

@shogosanz
Copy link
Contributor Author

shogosanz commented Jan 24, 2018

ご対応ありがとうございます!

lib/junoser/squash.rb だけでやる

すいません!ファイルを変更しました!

テストの追加

新しいコミットでテストを追加しました。
他のテストを見習って書いてみましたが、うまく書けているでしょうか?

追記 テスト結果をペーストしておきます。

Loaded suite test/test_junoser-squash
Started
..

Finished in 1.970141 seconds.
------------------------------------------------------------------------------------------------------
2 tests, 2 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
------------------------------------------------------------------------------------------------------
1.02 tests/s, 1.02 assertions/s

@codeout
Copy link
Owner

codeout commented Jan 24, 2018

ありがとうございます、よさそうなのでマージしますが、一点質問と一点お願いがあります。

  • exe/junoser-squash に実行属性ついてますか? (手元では 644 でした)
  • git rebase して1 commit にしていただけますか?

@shogosanz
Copy link
Contributor Author

おお!ありがとうございます!

exe/junoser-squash に実行属性ついてますか? (手元では 644 でした)

chmod 755致しました。

git rebase して1 commit にしていただけますか?

commit汚くなりまして申し訳ありません。
1commitにまとめました。

@codeout
Copy link
Owner

codeout commented Jan 24, 2018

パッチありがとうございました!

@codeout codeout merged commit f47145a into codeout:master Jan 24, 2018
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.

2 participants