I added a method to find a row in the value of the cell. #4
Conversation
| row.rowNum >= CLU.rowIndex(label) && | ||
| row.getCell(cellIndex)?.value && | ||
| row.getCell(cellIndex).isStringType() && | ||
| row.getCell(cellIndex).value.contains(cellValue) |
There was a problem hiding this comment.
row.getCell(cellIndex)が何度も登場しているのと、L171のvalue評価の時だけセーフナビゲーションがついているのがいまいちな感じですね。
There was a problem hiding this comment.
ああ、L171でnullの場合評価が終わるから後ろはセーフナビゲーションつけてない、ということですか。ううむ。
|
こちらも GExcelEditTest を削除して GExcelTest にテストを移しました。 |
| def book | ||
| def sheet | ||
| def sheet5 |
There was a problem hiding this comment.
#3と#4をとりあえずマージしてfixupしようと思ったのですが、編集箇所がかぶっていて2つめの自動マージでエラーになります。
同時に作成されている別のPR同士で前提状態を共有するのは好ましくないです。
PRは本来すべて独立にmerge or rejectできるようになっていなければなりません。
たとえば、このsheet5などは典型ですが、なぜ5なのかというと#3でsheet4を使っているからですよね。
(とはいえsheet2, sheet3は存在しないミッシングリンクになっているので、#3のsheet4自体が「なぜ4?」という感じでだいぶ状況は混乱していますね。基本的に「順番自体に大きな意味があるのでなければ通番は使わない」でください。)
ということで、解決方法としては、以下のどちらかかなと思います。
- (A) まず既存コードとの整合性を確保した上で(具体的には
sheet4って何？などとならないようにする。とりあえずPRしてからコメントを受けて修正する、でもいいですが。)、#3か#4のどちらかだけをPRとして作成する。それがmasterにマージされたら、そのmasterをベースに残りのブランチをrebaseしてからPRを作成する。 - (B) 今回の場合、
findRowという観点でみれば一連の機能と言える(特にempty版メソッドは値指定版メソッドの引数に空文字を指定しただけのエイリアスみたいなものと位置づけられる)ので、1つのPRにまとめる
今回の場合だと(B)でいい気がしてます。
元々1つだったPRを2つに分けたのにまた1つにするのー？と思われるかもしれませんが、元々の指摘では
また、一般的なPRに関する慣習として「1つのコミットにまとめてからPRする」というものがあります。こうすることで1PRにつき1観点(機能、バグフィックス、等)になり、管理がしやすくなります。これも是非お願いします。もし、複数の観点が混ざっているのであれば別のブランチとしてそれぞれ別のPRをにしてください。
のように「もし、複数の観点が混ざっているのであれば」という話です。
今回の場合「指定した条件に一致する行を特定するためのメソッドを新規追加するのだ」という点がハッキリしていると思うので、1つのPRでよいです。
I added a method to find a row in the value of the cell.
I have added test data and test cases for methods.