add set_order function #267

AnaTofuZ · 2018-02-26T06:38:49Z

目的

#228 である通り,orderの値をGoogleスプレッドシートを参照せず設定出来るようにする

行ったこと

~~dojo['order']に応じて値をsetするサブルーチンを定義しました~~
　- 当初はtaskとして切り出す事を考えていましたが、1手間かかることには変わりないので内部にいれました
~~環境変数 SET_JSONにGoogleAPIのjsonファイル~~
~~環境変数 SPREAD_SHEET_KEY にスプレッドシートのURLを設定すると使用可能です。~~
CSVファイルとしてスプレッドシートをローカルに落とす
CSVファイルから dojo['name'] に応じて都市名のorder を返す
該当しない、及び 都市 (地域) の記法出ない場合は例外を返す
すでにorder が決定している場合は何もしない
テストコード

困っている事

~~見つからなかった場合の処理~~
~~デプロイ時にjsonファイルを設定することが可能なのか~~
動きとしてこれで問題がないかどうか
dojos.rake では最初に order に応じてsortしている為に、適応順序の問題
政令指定都市を検索対象に含めるかどうか

dojo['order']に応じて値をsetするサブルーチンを定義しました。環境変数 SET_JSONにGoogleAPIのjsonファイルを、SPREAD_SHEET_KEYにスプレッドシートのURLを設定すると使用可能です。 - 見つからなかった場合の処理 - デプロイ時にjsonファイルを設定することが可能なのか - `dojos.rake` では最初に `order` に応じてsortしている為適応順序の問題

nalabjp · 2018-02-26T07:34:12Z

lib/tasks/dojos.rake

+
+    return name if  name =~ /^[0-9]+$/
+
+    conf = File.expand_path(ENV['SET_JSON'])


ENV['SET_JSON']がnilの場合、結果としてエラーになるのは一緒なのですが、
エラーの内容が全然違うものになるのでENV.fetch('SET_JSON')の方が良いかなーと思いました。

あと、SET_JSONって名前、もう少し直感的にどういうJSONファイルかが分かる名前だと良いなとおもいます。

ありがとうございます。 ENV.fetch だと例外が発生するんですね。
命名規則気を配ってきいます

nalabjp · 2018-02-26T07:34:20Z

lib/tasks/dojos.rake

+
+    conf = File.expand_path(ENV['SET_JSON'])
+    session = GoogleDrive:: Session.from_config(conf)
+    spred_sheet = session.spreadsheet_by_key(ENV['SPREAD_SHEET_KEY']).worksheets[0]


AnaTofuZ · 2018-02-26T07:40:56Z

こっちの PRではGoogleスプレッドシートにアクセスを行うように書いたのですがissue #228 の通り
一旦ローカルにCSVを落とし、そこから検索を行うという処理を行おうと思います。

またこれは質問なんですが、例外を発生させる時の良い感じの方法を教えていただきたいです。
今回は土地を検索しようとしているので、見つからなかった場合例外を発生させたいです

AnaTofuZ · 2018-02-26T07:41:36Z

また気づいたのですが名前は 鳥取 (中国) といった target (地域) の表記となるので正規表現を追加します

nalabjp · 2018-02-26T14:10:04Z

見つからなかった場合例外を発生させたいです

例外を発生させるのはraiseですねー。
https://docs.ruby-lang.org/ja/latest/class/Kernel.html#M_FAIL

resolve coderdojo-japan#228 でGoogleスプレッドシートにアクセスを行うとjsonファイルを別途生成するなどの必要性が出てきた。デプロイ環境でスプレッドシートにアクセス出来るかが怪しい為、スプレッドシートをローカルに落とし、実行出来るようにした. > また気づいたのですが名前は鳥取 (中国) といった target (地域) の表記となるので正規表現を追加しますここから、名前を正規表現で都市のみ抜き出して前文検索でorderを引くように設定した。

> こっちの PRではGoogleスプレッドシートにアクセスを行うように書いたのですがissue coderdojo-japan#228 の通り一旦ローカルにCSVを落とし、そこから検索を行うという処理を行おうと思います。現在スプレッドシートに上がっている全国地方公共団体コードの1ページを抜き取りcsv化した。文字コードの関係上 `nkf -Lu --overwrite local_public_organization.csv`を実行し、扱いやすいようにheaderを修正した。

AnaTofuZ

12行目dojos.sort_by!{ |hash| hash['order'] } の部分で order にもとづいてsort しているのですが、この処理を代替えすることが可能か検討しています。( order の値が決定しないとソートが不可能になっているため)

> 12行目dojos.sort_by!{ |hash| hash['order'] } の部分で order にもとづいてsort しているのですが、この処理を代替えすることが可能か検討しています。( order の値が決定しないとソートが不可能になっているため) 今回のソートは表示順ではなく保存の順序を決定しているのみであるので、削除しても動作上問題がないと考えられる為削除した。

yasulab · 2018-02-27T05:46:20Z

lib/tasks/dojos.rake

+
+    return pre_municipality if  pre_municipality =~ /^[0-9]+$/
+
+    if /(?<city>.+)\s\(.+\)/ =~ pre_municipality


municipality って言葉はほとんど聞かないので、townとかcity とかでどうですかね? 🤔

もしくは region とか。

ありがとうございます! city にrenameします

yasulab · 2018-02-27T05:49:43Z

lib/tasks/dojos.rake

+    return pre_municipality if  pre_municipality =~ /^[0-9]+$/
+
+    if /(?<city>.+)\s\(.+\)/ =~ pre_municipality
+      table = CSV.table(Rails.root.join('db','local_public_organization.csv'))


local_public_organization.csv

このファイル名だと分かりづらいですね 😓 city_code.csv とか city_to_code.csv とかどうでしょう? 🤔

> municipality って言葉はほとんど聞かないので、city でどうですかね? 🤔 > このファイル名だと分かりづらいですね 😓 city_code.csv とかcity_to_code.csv とかどうでしょう? 🤔 命名が分かりづらかった為,レビューを反映しました。 csvファイルの該当カラムも同時に命名を変更しています

AnaTofuZ · 2018-02-27T06:14:57Z

レビューを反映して、変数名を修正しました!

yasulab · 2018-02-27T06:18:48Z

良さそう! @nalabjp さんのレビューを待ってからマージしましょう ;)

nalabjp · 2018-02-27T06:35:41Z

lib/tasks/dojos.rake

@@ -18,20 +19,20 @@ namespace :dojos do
      dojo.delete 'updatedAt' # This is managed by database
    end

+


不要な改行ですかね？

nalabjp · 2018-02-27T06:43:17Z

lib/tasks/dojos.rake


    dojos.each do |dojo|
      d = Dojo.find_or_initialize_by(id: dojo['id'])

      d.name        = dojo['name']
      d.email       = ''
-      d.order       = dojo['order']
+      d.order       = set_order( dojo['order'] ?  dojo['order']: dojo['name'])


dojo['order']があればdojo['order']を、なければdojo['name']をset_orderの引数に使用する。
その上で、L52の実装により、
引数に渡ってきた値が数値のみの場合はそのまま返却する、ということであれば、
dojo['order'] || set_order(dojo['name'])とするほうが意図が明確でメソッドコールもなくなるので良さそうだなと思いました。
あわせてL52の実装については不要になるかなと思います。

nalabjp · 2018-02-27T06:44:44Z

lib/tasks/dojos.rake

@@ -44,6 +45,21 @@ namespace :dojos do
    end
  end

+  # search order number for google spred sheets
+  # 'yamlファイルのnameからorderの値を生成します'
+  def set_order(pre_city)


setしているわけではないのでsearch_order or search_order_numberの方が直感的かなと思いましたがどうでしょうか？

nalabjp · 2018-02-27T06:45:33Z

lib/tasks/dojos.rake

+  # 'yamlファイルのnameからorderの値を生成します'
+  def set_order(pre_city)
+
+    return pre_city if  pre_city =~ /^[0-9]+$/


上でコメントしたとおりdojo['name']しか渡ってこないなら不要ですかね？

nalabjp · 2018-02-27T06:57:42Z

lib/tasks/dojos.rake

+    if /(?<city>.+)\s\(.+\)/ =~ pre_city
+      table = CSV.table(Rails.root.join('db','city_code.csv'))
+      row = table.find{ |r| r[:city].to_s.start_with?(city)}
+      row ? row[:order] : raise("Can't searched order by #{pre_city}")


僕も英語が得意なわけではないので自信ないんですが、
「検索できなかった」と過去形にしたいなら「Couldn't search」の方だったりしますかね？
ちょっと違和感があったのでgoogle翻訳に入れてみたら「もしかして Can't search 〜」と言われたので😅
よくある語彙としては「Not found」かなーと思うのでそれでも良いかなと思いました😌

nalabjpさんのレビューを反映しました > dojo['order'] || set_order(dojo['name'])とするほうが意図が明確でメソッドコールもなくなるので良さそうだなと思いました。 > あわせてL52の実装については不要になるかなと思います。確かにメソッドコールのコストが掛からなくなる為修正しましたメソッド内部の判定部分も不要になるため削除しましたまたsearch_order_numberに名前を修正しています。

AnaTofuZ · 2018-02-27T07:16:38Z

@nalabjp さんのレビューを反映しました!
レビューありがとうございます!!

nalabjp

LGTM 👀 👍
good works 🎉

AnaTofuZ · 2018-02-27T08:14:15Z

ありがとうございます! お二人かapproveがきていたのでmergeします

yasulab · 2018-02-27T09:15:13Z

Nice work!! 👍✨

yasulab · 2018-02-27T10:21:40Z

今回のマージでorder 周りの設定手順が変わったと思うので、 docs/how-to-add-dojo.md のドキュメントにも反映しておくと良いかも 🤔💭

Fix coderdojo-japan#268 > 今回のマージでorder 周りの設定手順が変わったと思うので、docs/how-to-add-dojo.md のドキュメントにも反映しておくと良いかも > cf. coderdojo-japan#267 `order`はnameから取っている為、市町村と同じDojo名だと省略可能な旨を追記した

coderdojo-japan#267 で`order` を自動追加出来るように設定していたが yamlファイルを出力するタイミングで,orderを出力するようにした

nalabjp reviewed Feb 26, 2018

View reviewed changes

AnaTofuZ added 2 commits February 27, 2018 10:21

AnaTofuZ commented Feb 27, 2018

View reviewed changes

AnaTofuZ changed the title ~~[WIP]add set_order function~~ add set_order function Feb 27, 2018

yasulab reviewed Feb 27, 2018

View reviewed changes

yasulab approved these changes Feb 27, 2018

View reviewed changes

nalabjp reviewed Feb 27, 2018

View reviewed changes

nalabjp approved these changes Feb 27, 2018

View reviewed changes

AnaTofuZ merged commit ca6865d into coderdojo-japan:master Feb 27, 2018

AnaTofuZ deleted the auto_order_number branch February 27, 2018 08:14

yasulab mentioned this pull request Feb 27, 2018

docs/how-to-add-dojo.md を更新する #268

Closed

AnaTofuZ mentioned this pull request Feb 28, 2018

add notes order #269

Merged


		return name if name =~ /^[0-9]+$/

		conf = File.expand_path(ENV['SET_JSON'])


		return pre_municipality if pre_municipality =~ /^[0-9]+$/

		if /(?<city>.+)\s\(.+\)/ =~ pre_municipality

		@@ -18,20 +19,20 @@ namespace :dojos do
		dojo.delete 'updatedAt' # This is managed by database
		end

Uh oh!

add set_order function #267

add set_order function #267

Conversation

AnaTofuZ commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

目的

行ったこと

困っている事

Uh oh!

nalabjp Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnaTofuZ commented Feb 26, 2018

Uh oh!

AnaTofuZ commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nalabjp commented Feb 26, 2018

Uh oh!

AnaTofuZ left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yasulab Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnaTofuZ commented Feb 27, 2018

Uh oh!

yasulab commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnaTofuZ commented Feb 27, 2018

Uh oh!

nalabjp left a comment

Choose a reason for hiding this comment

Uh oh!

AnaTofuZ commented Feb 27, 2018

Uh oh!

yasulab commented Feb 27, 2018

Uh oh!

yasulab commented Feb 27, 2018

Uh oh!

Uh oh!

AnaTofuZ commented Feb 26, 2018 •

edited

Loading

nalabjp Feb 26, 2018 •

edited

Loading

AnaTofuZ commented Feb 26, 2018 •

edited

Loading

AnaTofuZ left a comment •

edited

Loading

yasulab Feb 27, 2018 •

edited

Loading