Skip to content
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

replace create.py to install ckan command create table #6

Merged
merged 58 commits into from Feb 15, 2023

Conversation

To-Ki-O
Copy link
Collaborator

@To-Ki-O To-Ki-O commented Feb 1, 2023

create.pyファイルを置き換えました。

以下のようにコマンドを作成しました。

create table (option) で各テーブルを作成できます。

  • -u, --utilization

    • utilization, utilization_summary, utilization_feedback, issue_resolution, utilization_feedback_reply が作成できます。
  • -r, --resource

    • resource_feedback, resource_feedback_reply が作成できます。
  • -c, --count

    • resource_summary が作成できます。
  • -A, -alltables

    • 全てのテーブルを作成できます。
  • ENUMを用いているところは"genre1", "genre2"として仮置きしています。

  • ckan内で動作確認済みです。

@ryo-ma
Copy link
Contributor

ryo-ma commented Feb 1, 2023

コマンドの設計について

  • -u-r-c-Aと並べられたときに一般的なエンジニア(少なからず私)はそれぞれが全く別の独立した機能のオプションに見えます。たとえば-cはcountなので何か数えたものを結果で出力するオプションなのかと思いました。
  • create table (option)とのことですがoptionがなかった場合のdefaultの動作はどうなるのでしょうか? optionが必須のようにみえ、defaultの動作説明がないようなので質問いたしました。
  • この辺りのパラメータは外から渡せるようにして基本コードの中には書かないようにすべきです。たとえば設定ファイル(xx.ini ?)、環境変数、これこそコマンドのオプション (どうしてもCKAN の仕様上仕方ないのであれば要検討ですが)
    • DB_HOST
    • DB_PORT
    • DB_NAME
    • DB_USER
    • DB_PASS

とりあえず有名なコマンドの設計などを参考にしてみてください。ちゃんと世の中のエンジニアに合わせてユーザが直感的に使える物を目指してください。

@To-Ki-O
Copy link
Collaborator Author

To-Ki-O commented Feb 1, 2023

  • オプションについて
    承知いたしました。他のコマンド設計を参考に考え直してみます。

  • default の動作について
    default で実行は可能ですが、特に何も起こらずに動作が終了します。
    オプションを指定するようにエラーを出すのか、オプションではなくコマンドで動作を分けるのか
    といったところも他のコマンドを見て考えてみます。

  • DBの情報について
    承知いたしました。
    設定ファイルなどを見直してみます。

@ryo-ma
Copy link
Contributor

ryo-ma commented Feb 1, 2023

default の動作について
default で実行は可能ですが、特に何も起こらずに動作が終了します。
オプションを指定するようにエラーを出すのか、オプションではなくコマンドで動作を分けるのか
といったところも他のコマンドを見て考えてみます。

なるべく可能な限りオプションはシンプルにすべきです。必須パラメータがあるなら説明に必須と明示的に書くべきです。

まず、実装する前にコマンドラインの設計をこちらに記載してください。

@To-Ki-O
Copy link
Collaborator Author

To-Ki-O commented Feb 1, 2023

承知しました。

@To-Ki-O
Copy link
Collaborator Author

To-Ki-O commented Feb 2, 2023

ckan command

create table

デフォルト

【動作】全てのテーブルを作成する

オプション:-s [ util, review, num_use ]

* create table -s util
	【関連する機能】利活用方法、利活用方法へのレビュー、課題解決数
	【動作】utilization, utilization_feedback, utilization_feedback_reply, utilization_summary, issue_resolution の5つのテーブルを作成する

* create table -s review
	【関連する機能】データへのレビュー
	【動作】resource_feedback, resource_feedback_reply の2つのテーブルを作成する

* create table -s num_use
	【関連する機能】ダウンロード数、利活用数
	【動作】resource_summary のテーブルを作成する

Linuxコマンドにおいて、-s は何かを指定する際に用いられていたため、作成する機能・テーブル群を指定するという意味で選択しました。

@ryo-ma
Copy link
Contributor

ryo-ma commented Feb 2, 2023

  • -s は何の略でしょうか? 以下の理由にあまりピンとこないのですが、一般的であるならば具体的に 「-s で何かを指定」をするコマンドの例をいくつか教えていただけますでしょうか?

Linuxコマンドにおいて、-s は何かを指定する際に用いられていたため

  • 2つの機能を指定したい場合も想定したいのですが、複数指定する場合はどうなっておりますでしょうか?

  • optionはこれだけでしょうか?HOSTとかの件はどうなりましたか?

  • ckanへ追加されていくコマンドの名前空間に関して気になったので質問です。createのようなありきたりなコマンド名だと他のコマンドと被ったりすることはないんですかね?

@To-Ki-O
Copy link
Collaborator Author

To-Ki-O commented Feb 2, 2023

-s について

例としては以下のものがありました。

  • whatis -s [リスト]:検索対象の章番号を指定
  • apropos -s [リスト]:検索対象の章番号を指定
    ソートに関するオプションでは -S と大文字になっておりました。

想像で select だと思っておりましたが、sections の s をとったものでした。
再度、Host の件と合わせて考え直します。
また、Hostにつきましては各環境変数がどこにあるかを調査している段階になります。

複数指定について

create table -s util review のように後ろに繋げることで複数指定ができるようにするイメージをしておりました。

名前空間について

Ckan が持つデフォルトのコマンドでは create は存在していないので利用できます。
他のextensionで ckan command を追加するものがあれば、重複してしまう可能性はあります。
ckan command を利用するextensionは調査をした方が良いでしょうか?

@ryo-ma
Copy link
Contributor

ryo-ma commented Feb 2, 2023

想像で select だと思っておりましたが、sections の s をとったものでした。

sectionだと記述通り章という意味になってしまいますね。

また、Hostにつきましては各環境変数がどこにあるかを調査している段階になります。

環境変数がどこにあるかにしろ、コマンドから指定できるようにはしておいた方がいいので追加してください

Ckan が持つデフォルトのコマンドでは create は存在していないので利用できます。
他のextensionで ckan command を追加するものがあれば、重複してしまう可能性はあります。
ckan command を利用するextensionは調査をした方が良いでしょうか?

feedbackのExtensionに関連するコマンドであることは明示的にわかるようにしておいた方がいい気がしてきました。
他のExtensionがどういう名前にしているのかも気になります。

ちなみに実際に叩く際はどのような形になるんでしたっけ?以下のように頭にckanをつけるんでしたっけ?

$ ckan create table xxxx

@To-Ki-O
Copy link
Collaborator Author

To-Ki-O commented Feb 2, 2023

承知しました。

他のextensionでコマンドを追加しているものを調査してみます。

実際にコマンドを入力する際は以下のように production.ini の場所を指定する必要があります。
ckan --config=/etc/ckan/production.ini create table xxxx

@To-Ki-O
Copy link
Collaborator Author

To-Ki-O commented Feb 2, 2023

開発する CKAN command 仕様

デフォルト

ckan feedback init
  • ckanext-feedback で利用する全てのテーブルを作成する
  • ckanext-feedback に必要な初期設定を行う

オプション

--modules -m

  • オンオフ機能と連動して、必要になるテーブルを作成する
  • 現在は以下のように分割したものを想定している
    • utilization
    • review
    • download
      ※この分割で実装が可能か要検討

例)

ckan --config=/etc/ckan/production.ini feedback init -m utilization review

utilization(利活用方法)とreview(データへのレビュー)に関連したテーブル作成を行う

--host -h

  • Postgresql の接続に使うホスト名をコマンド上から指定する際に利用する

--port -p

  • Postgresql の接続に使うポート番号をコマンド上から指定する際に利用する

--name -n

  • Postgresql の接続に使うデータベース名をコマンド上から指定する際に利用する

--user -u

  • Postgresql の接続に使うユーザー名をコマンド上から指定する際に利用する

--password -pw

  • Postgresql の接続に使うパスワードをコマンド上から指定する際に利用する

@To-Ki-O
Copy link
Collaborator Author

To-Ki-O commented Feb 7, 2023

出力について

デフォルトのckanコマンドに合わせる形で出力を考えました。

デフォルト実行(ckan feedback init)

  • 成功時

    • Initialization: success(緑色太字)を表示
  • 失敗時

    • エラー文を表示

モジュールのオプション選択時(ckan feedback init -m xxxxx)

  • 成功時

    • -m utilization指定時
      • Initialize utilization: SUCCESS(緑色太字)を表示
    • -m review指定時
      • Initialize review: SUCCESS(緑色太字)を表示
    • -m download指定時
      • Initialize download: SUCCESS(緑色太字)を表示
  • 失敗時

    • エラー文を表示

よろしくお願いします。

@To-Ki-O
Copy link
Collaborator Author

To-Ki-O commented Feb 7, 2023

修正点

出力

  • デフォルト入力(ckan feedback init)時の出力について
    • initialize all modules: SUCCESS(緑色太字)に変更いたしました。

動作

  • べき等性について

    • commit()の位置を変更し、エラー発生時にコミットされないようにいたしました。
  • 環境変数

    • 環境変数が設定されている時は環境変数の値を利用する形にしました。
    • 環境変数が設定されていない時はデフォルトの値としてckanのデフォルト値を利用しています。
    • また、オプションで指定することも可能です。

その他

  • SQL文について

    • テーブル設計が固まり次第、更新を考えています。
    • 現状は以前のテーブル設計を参考にSQL文を構築しています。
  • ENUMの値について

    • こちらも値が決まり次第、更新いたします。
    • 現在は仮置きの値となっております。

よろしくお願いします。

development/misc/feedback.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
@To-Ki-O
Copy link
Collaborator Author

To-Ki-O commented Feb 8, 2023

修正点

オプション名について

  • --passwordオプションの短縮形を-Pに変更しました。

処理について

  • 以下の順で処理を行うように変更いたしました。
    • 全てのテーブルを削除(clean)
    • 指定のテーブル作成コマンドを実行
    • 書き込み処理(commit)

べき等性について

  • エラーログ出力後、プログラムを終了するように変更いたしました。(sys.exit())
  • utilizationdownloadを指定し、downloadでエラーが発生した場合はcleanutilizationの実行はなかったこととなり、コマンド実行前の状態に戻ることを確認しました。

development/misc/feedback.py Outdated Show resolved Hide resolved
@ryo-ma
Copy link
Contributor

ryo-ma commented Feb 8, 2023

本cliで今決まっている仕様の部分だけでものドキュメントをcliのディレクトリにREADME.mdとして記載してください。ルートのREADME.mdからリンクさせる予定です。

@To-Ki-O
Copy link
Collaborator Author

To-Ki-O commented Feb 8, 2023

README.md作成の件、承知いたしました。

development/misc/README.md Outdated Show resolved Hide resolved
development/misc/README.md Outdated Show resolved Hide resolved
development/misc/README.md Outdated Show resolved Hide resolved
development/misc/cli.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
development/misc/feedback.py Outdated Show resolved Hide resolved
@Sheile
Copy link
Contributor

Sheile commented Feb 14, 2023

@To-Ki-O
細かい点はありますが、 #7 で対応するものもあるため、 #6 はこの対応で良いと思います。

@ryo-ma
全てResolveになりましたので、PRのマージをお願いします。

@ryo-ma
Copy link
Contributor

ryo-ma commented Feb 15, 2023

@Sheile 承知しました。ありがとうございます。一旦このPRはこれでマージいたします。

SQLAlchemy対応に関しては別PRで実施する予定です。

@ryo-ma ryo-ma merged commit 0a530b9 into c-3lab:main Feb 15, 2023
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.

None yet

3 participants