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: add precommit #59

Merged
merged 16 commits into from
Nov 20, 2024
Merged

ci: add precommit #59

merged 16 commits into from
Nov 20, 2024

Conversation

asa-naki
Copy link
Contributor

@asa-naki asa-naki commented Nov 18, 2024

description

#54
のPRを取り込むと、udp通信に失敗するとundefined nameで実行時エラーとなってしまうことが分かった。
実行時のクリティカルなエラーとなりうるソースコードを入れないようにそのため静的解析ツールを導入して、検知をできるようにする。

導入する静的解析ツールとして、autowareが採用していて、実績があることから、flake8 rosを導入する。

起動設定として、github actionsの PR作成された時、及びPRの変更がpush された時にワークフローが実行されるように作成した。

  • .github/workflows/pre-commit.yaml
  • .pre-commit-config.yaml

本PRでは、実行時エラーに紐付かない(クリティカルではない)エラーの抑制したい。
https://flake8.pycqa.org/en/latest/user/configuration.html#project-configuration に準拠して、
setup.cfgに設定を追加する。

setup.cfgについて

現在のv2i_interfaceでのソース上でflake8を実行させると、エラーを検出している。
ここで発生したエラーコードの一覧を下表に示す。

下表のエラーコードの一覧の中からエラーとして検出すべきものを選定した。
当初の要求通り、実行時エラーが発生するものだけをエラー検出対象とする。

コード 概要 要否
A001 A variable is shadowing a Python builtin X
A002 argument "id" is shadowing a python builtin X
CNL100 Class definition does not have a new line X
D100 Missing docstring in public module X
D101 Missing docstring in public class X
D102 Missing docstring in public method X
D103 Missing docstring in public function X
D104 Missing docstring in public package X
D105 Missing docstring in magic method X
D107 Missing docstring in init X
E203 Whitespace before ':' X
E225 Missing whitespace around operator X
E231 Missing whitespace after ',', ';', or ':' X
E275 Missing whitespace after keyword X
E301 Expected 1 blank line, found 0 X
E302 Expected 2 blank lines, found 0 X
E305 Expected 2 blank lines after end of function or class X
E501 Line too long (82 > 79 characters) X
E712 Comparison to true should be 'if cond is true:' or 'if cond:' X
F401 Module imported but unused X
F821 Undefined name name O
I100 Import statements are in the wrong order. X
I201 Missing newline between import groups. X
Q000 Double quotes found but single quotes preferred X
W291 trailing whitespace X

test

https://github.com/eve-autonomy/v2i_interface/actions/runs/11925967465/job/33238970414?pr=59
でflake8 のgithub workflow が走っていることを確認済み。

https://github.com/eve-autonomy/v2i_interface/actions/runs/11925967465/job/33238970414?pr=59
actionsで実行時クリティカルなエラーとなる、F821を検知できていることを確認済みです。

@asa-naki asa-naki changed the title Ci/add precommit ci: add precommit Nov 18, 2024
@asa-naki asa-naki marked this pull request as ready for review November 18, 2024 09:29
@sfukuta
Copy link
Contributor

sfukuta commented Nov 18, 2024

ローカルの結果はありますが、Actionsからテスト実行できないというのはあっています?
そうであれば、その旨記載して、マージ後に最終確認を実施としてほしいです。

@asa-naki
Copy link
Contributor Author

asa-naki commented Nov 18, 2024

Actionsからテスト実行できないというのはあっています?
そうであれば、その旨記載して、マージ後に最終確認を実施としてほしいです。

@sfukuta
workflowの設定次第でした。
ef0243a
で修正してみたところ、テスト実行ができました。

@asa-naki asa-naki requested a review from yn-mrse November 19, 2024 02:23
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@masahiro-kubota
Copy link
Contributor

@asa-naki
https://github.com/tier4/autoware.universe/blob/beta/v0.3.20/pre-commit-config.yaml
description内の上記リンクが切れてしまっています。

@masahiro-kubota
Copy link
Contributor

https://github.com/tier4/autoware.universe/blob/beta/v0.3.20/
こちらも切れてしまっているようです。

@asa-naki
Copy link
Contributor Author

@masahiro-kubota
リンク切れの指摘ありがとうございます!
修正しました!

@peeeechi
Copy link

peeeechi commented Nov 20, 2024

現在出力されるflake8 のエラーについて、

  • F系以外ではクリティカルなものは無い
  • F系でも F401はクリティカルではない
    と判断していると認識しました。
    ここはOKです!

起動設定として、github actionsの毎回のpushでワークフローが実行されるように作成した。

今回の .github/workflows/pre-commit.yaml を確認すると、on: push では無く on: pull_request となっているかと思います。
上記の記載内容は on: pushの際の挙動であり、 on: pull_request の際の挙動は "PR作成された時、及びPRの変更がpush された時"になります。
記載内容が的確でなく、且つ誤解を生む表現ですので、この部分のみDescripton の修正をお願いします
(on: pull_requestの設定は問題ないです。)

@asa-naki
Copy link
Contributor Author

asa-naki commented Nov 20, 2024

@peeeechi
↑の指摘について、修正しました!

@asa-naki asa-naki merged commit 44f8f56 into main Nov 20, 2024
1 check failed
@asa-naki asa-naki deleted the ci/add_precommit branch November 20, 2024 11:08
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.

5 participants