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

CI: reviewdog 導入 #458

Merged
merged 6 commits into from
Oct 30, 2016
Merged

CI: reviewdog 導入 #458

merged 6 commits into from
Oct 30, 2016

Conversation

haya14busa
Copy link
Member

ref: #454

@@ -4,6 +4,8 @@ let s:save_cpo = &cpo
set cpo&vim

function! s:_vital_loaded(V) abort
let unused = 1
let dq = "double quote"

Choose a reason for hiding this comment

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

reported by reviewdog 🐶
Prefer single quoted strings (see Google VimScript Style Guide (Strings))

@thinca
Copy link
Member

thinca commented Oct 29, 2016

将来的には私も vint 使いたいと思っているんですが、現状では vimlint を使っているので、vimlint にできないでしょうか? (Vim のインストールが必要なのでめんどいのはわかる)

@haya14busa
Copy link
Member Author

両方できるかテストしようとしてとりあえずvintにしてましたがvimlint実行できるようにしますー
(最初にvint試したのはVim のインストールが必要で面倒だったというのはまさにという感じですが)

@haya14busa
Copy link
Member Author

あと dockerfile どっかに置かないとだめなんで vim-jp に置くかしていただけますかね? (まだdockerfile 完成してないけど)

というか vital.vim repo においてもいいのかな.うーむdocker力...

@@ -4,6 +4,8 @@ let s:save_cpo = &cpo
set cpo&vim

function! s:_vital_loaded(V) abort
let unused = 1
let dq = "double quote"

Choose a reason for hiding this comment

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

[vint] reported by reviewdog 🐶
Prefer single quoted strings (see Google VimScript Style Guide (Strings))

Copy link
Member Author

Choose a reason for hiding this comment

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

[補足] --name=vint 機能をreviewdogに追加したので新たにコメントがついてますが,本来は二重でコメントされることはありません

@@ -4,6 +4,8 @@ let s:save_cpo = &cpo
set cpo&vim

function! s:_vital_loaded(V) abort
let unused = 1
let dq = "double quote"

Choose a reason for hiding this comment

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

[vimlint] reported by reviewdog 🐶
EVL102: unused variable l:dq

@@ -4,6 +4,8 @@ let s:save_cpo = &cpo
set cpo&vim

function! s:_vital_loaded(V) abort
let unused = 1

Choose a reason for hiding this comment

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

[vimlint] reported by reviewdog 🐶
EVL102: unused variable l:unused

@haya14busa
Copy link
Member Author

haya14busa commented Oct 29, 2016

droneioは実行環境としてdocker image を使うのですがとりあえず haya14busa/dockerfile-lint-vital.vimにおいてみています. https://github.com/haya14busa/dockerfile-lint-vital.vim/blob/master/Dockerfile

vim-jp リポジトリに移すか, vital.vim において自動でdockerhubにあげるようにtravisかcirle ci でCIすると良いかも知れない.ご意見募集です
travisとかcircle ci 使わなくても docker hub の連携で dockerfile の場所指定できそうなので vital.vim リポジトリに置くとよいかなー.dockerhub 連携はおそらく vital.vim の管理者権限が必要っぽいので,vital.vimに置きたい場合はそのへんよろしくお願いしたい

@@ -0,0 +1,15 @@
# Run the below command when you edit .drone.yml
# drone secure --repo vim-jp/vital.vim --in .drone.sec.yaml
Copy link
Member Author

Choose a reason for hiding this comment

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

悪意あるp-rが yml ファイルを書き換えることによってGitHub の API token が漏らされることを防ぐため,コメントにあるコマンドを.drone.ymlファイルを変更するごとに実行する必要があります.

これを実行するにはもちろん API token を知っている (手元に .drone.sec.yaml) がある必要があり,現状僕とthincaさんが出来るようになっています.vital.vim メンテナの人には thinca さんとも相談の上権限を渡していくとよいかなと思っています

@haya14busa
Copy link
Member Author

drone.io 参考doc

とりあえずこんな感じかなというとこまでやったのでレビューよろしくお願いします.
この dockerfile も含む haya14busa/dockerfile-lint-vital.vim@389f3ab

vint も現在実行してみちゃってるけど消してもよいという感じです.

@haya14busa haya14busa changed the title [WIP] CI: reviewdog 導入 CI: reviewdog 導入 Oct 29, 2016
Copy link
Member

@thinca thinca left a comment

Choose a reason for hiding this comment

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

LGTM ✨

@thinca
Copy link
Member

thinca commented Oct 30, 2016

全体的には OK です。Dockerfile の扱いどうしますかねー。
この Dockerfile、vital.vim 用かと思いきや別にそういうわけではないんですよね。となると vital.vim のリポジトリに入れちゃうかは悩ましい(専用と割り切るのも手だけど)。
あとで考えるってのでも良ければ、とりあえずこれはマージしちゃいますが、どうしましょ?

@lambdalisue
Copy link
Member

🎉

@haya14busa
Copy link
Member Author

haya14busa commented Oct 30, 2016

確かに完全に専用ではないんですよねー.ただ,vital.vim リポジトリで他のlintを走らせたくなったときは基本的にdrone.yml内で頑張ってtool用意するよりdockerfileで用意してしまうほうがいい感じなので悩ましい.

vim/vint/vim-vimlint (/reviewdog) が入った docker image をbase にしたvital.vim用のimageがあるのが理想かもしれない...?

まぁとはいえあとで考えるというのでよいのであればそれでお願いしたいかもです.現状他に入れたいlinterもないし.

あとは現状 vim-vimlint も vint も両方実行しているので,場合によってはコメントが2つつくケースがあると思うので .vintrc.yaml あたりを導入して vim-vimlint でやっているやつはdisableするとよさそう.
これはこのp-rである程度やっても良いかも知れないけど,クリティカルな問題でもないので別p-rでやらせてもらっても便利という感じですかね

@thinca thinca merged commit 6e682fc into vim-jp:master Oct 30, 2016
@thinca
Copy link
Member

thinca commented Oct 30, 2016

ひとまずマージしましたっ!
何かに使えるかもと思い、Docker Hub に vim-jp 用の org を勢いで作ってみました。具体的にどう使うかは一切考えていない。必要であれば invite します。
https://hub.docker.com/u/vimjp/
org 名には小文字アルファベットと数字しか使えないというキビシイ制約があったので、vimjp という名前になってしまった。

@haya14busa haya14busa deleted the reviewdog branch October 30, 2016 13:54
@haya14busa
Copy link
Member Author

マッジありがとうございます! 🎉

vimjp DockerHub べんりそう

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.

4 participants