Skip to content
This repository has been archived by the owner on Aug 17, 2023. It is now read-only.

statusコマンドの実装 #170

Merged
merged 6 commits into from
Feb 6, 2019
Merged

statusコマンドの実装 #170

merged 6 commits into from
Feb 6, 2019

Conversation

altnight
Copy link
Contributor

@altnight altnight commented Jan 31, 2019

チケットURL

このレビューで確認してほしい点

  • 全体的に不明な記述がないか
  • クリティカルな問題がなさそうか

レビューチェックリスト

  • C2 体を表す名前の公理:あらかじめ決められている以外の汎用的な名前のモジュールを作らない
  • C3 汎用名のモジュール内に長々と具体的処理を書かない
  • C4 単純な処理の長さで分割しない
  • C5 引数の数を減らす
  • C6 パッケージ間で共通した定数を作らない
  • C7 継承の利用を最小限にする
  • C8 親クラスのテストを子クラスでも実行すること
  • C9 オーバーライドを減らす
  • C10 継承やオーバーライドを明示する

@altnight altnight self-assigned this Jan 31, 2019
@altnight altnight changed the title [WIP] statusコマンドの実装 statusコマンドの実装 Feb 6, 2019
@altnight
Copy link
Contributor Author

altnight commented Feb 6, 2019

@だれか レビューお願いします

Copy link
Member

@takanory takanory left a comment

Choose a reason for hiding this comment

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

LGTM with nitsです

show_resources(message)


@respond_to('^status\s+del\s+(\S+)$')
Copy link
Member

Choose a reason for hiding this comment

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

nits: (del|delete|rm|remove) にして、2番目の引数捨てるほうがいいのでは

Copy link
Contributor Author

Choose a reason for hiding this comment

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

あ、たしかにそういう記述方法ありますね。そのほうが見やすいので変えます 👍

if resource:
s.delete(resource)
s.commit()

Copy link
Member

Choose a reason for hiding this comment

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

nits: 削除しました的なメッセージがあると親切

Copy link
Contributor Author

Choose a reason for hiding this comment

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

操作ごとのメッセージは考えたんですが、常に最新の状態一覧を返しているのでないほうが簡潔かなーというポリシーです

status=None,
))
s.commit()

Copy link
Member

Choose a reason for hiding this comment

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

nits: 追加しました的なメッセージがあると親切。 show_resources に追加のメッセージとか渡せるといいかも

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(削除と同じく)

@altnight altnight merged commit 466c9f2 into master Feb 6, 2019
@altnight altnight deleted the t169 branch February 6, 2019 09:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants