search

sbgisen / .github

GitHub meta repository for sbgisen.
0 stars 4 forks source link

PEP8準拠の命名規則がreviewdogでチェックされない #46

Closed Tacha-S closed 2 years ago

Tacha-S commented 2 years ago

概要

クラス名はCamelCase、関数名はsnake_caseといったPEP8で定められている命名規則がreviewdogのflake8では引っかからない

例

#!/usr/bin/env python
# -*- coding:utf-8 -*-

class foo_bar(object):
    def __init__(self):
        pass
flake8 test.py
test.py:4:8: N801 class name 'foo_bar' should use CapWords convention

再現手順

命名規則のおかしいコミットをCIにチェックさせる

修正しないとどう困るか

命名規則を人力でチェックしないといけない

原因

reviewdogがpep8-namingを入れていない。 or N***をスルーする

修正案 N***を拾ってreviewdog経由でreview commentつけるstepを追加する or reviewdogで拾えるようにする。

Tacha-S commented 2 years ago

flake8のpluginとして有用そうなもの

Tacha-S commented 2 years ago

GitHub Actionで入れられているflake8のpluginはデフォルトで入るもののみ

[action-flake8] Flake8 version:
3.9.2 (mccabe: 0.6.1, pycodestyle: 2.7.0, pyflakes: 2.3.1) CPython 2.7.18 on
Linux
h-wata commented 2 years ago

flake8のpluginとして有用そうなもの

  • pep8-naming

    • 命名規則をチェック

  • flake8-import-order

    • importの順序や、grouping(標準ライブラリ, 3rd party, local)をチェック
    • styleもいくつか選べる

  • flake8-docstrings

    • docstringsがついているかのチェック(styleもpep257, numpy. googleから選べるらしい)

  • flake8-coding

    • # -*- coding:utf-8 -*- のようなencodingの指定のチェック(python3になったらいらないけどpython2の間は必要)

  • flake8-copyright

    • copyrightがあるかのチェック(styleは正規表現用意しないといけない)

  • flake8-quotes

    • "にする必要のないstringを'にするようにチェックまたはその逆

どれも有用そうですね。 どれも入れて良いと思います。他で必要そうなのは自分も探してみます。

h-wata commented 2 years ago

flake8では無いですが、mypyで静的に型チェックをしてくれるみたいです。

reviewdogもあります。 https://github.com/tsuyoshicho/action-mypy Github https://github.com/python/mypy ※VScode, Atomのプラグインもありそう。

Tacha-S commented 2 years ago

mypyはpython3で入っているtype-hintingがあると使いやすいと思うんですが、 python2だとコメントで型を明示的にしてあげないといけないので若干使いにくい気がします。 https://mypy.readthedocs.io/en/stable/python2.html

MikhailBertrand commented 2 years ago

flake8-docstrings

これドキュメント推奨派の私としてはすごく導入したいのですが、現在うちで管理されている多くのコードにドキュメントが無いので今後はPR送る人がその都度追加する感じになるんですかね？

flake8-copyright

うちのライセンス表記は決められているものの、あまり記述されていないように見受けられるのでcubeリポジトリをはじめとしてどこかのタイミングで一斉に導入出来たらいいなと思っています。 https://github.com/sbgisen/sbgisen_wiki/wiki/Git:%E3%82%BD%E3%83%BC%E3%82%B9%E3%82%B3%E3%83%BC%E3%83%89%E3%81%AE%E3%83%A9%E3%82%A4%E3%82%BB%E3%83%B3%E3%82%B9%E3%81%AB%E3%81%A4%E3%81%84%E3%81%A6

h-wata commented 2 years ago

そうなんですね。勉強になりました。 コメントというのは下記ですかね。確かに、# type:を追加していくのは面倒ですね。 https://www.python.org/dev/peps/pep-0484/#suggested-syntax-for-python-2-7-and-straddling-code

了解です。

MikhailBertrand commented 2 years ago

他だとF841 local variable 'hoge' is assigned to but never usedとか今のreviewdogでは引っ掛かっていないみたいですね。

MikhailBertrand commented 2 years ago

https://github.com/DmytroLitvinov/awesome-flake8-extensions

Tacha-S commented 2 years ago

うちのライセンス表記は決められているものの、あまり記述されていないように見受けられるのでcubeリポジトリをはじめとしてどこかのタイミングで一斉に導入出来たらいいなと思っています。

formatがあるのに書いてないことがほとんどなのでほぼコピペで済みますし入れたいですよね。

これドキュメント推奨派の私としてはすごく導入したいのですが、現在うちで管理されている多くのコードにドキュメントが無いので今後はPR送る人がその都度追加する感じになるんですかね？

これは確かに問題で、ドキュメントのないファイルの変更し損と思われるとよくないですよね。 案としてはドキュメントに関してはしばらくの間指摘はされるけどCIはパスするのはありかもしれないです。

コメントというのは下記ですかね。確かに、# type:を追加していくのは面倒ですね。

それですね docstringとは別に用意しなければいけないのが少し残念です。

Tacha-S commented 2 years ago

他だとF841 local variable 'hoge' is assigned to but never usedとか今のreviewdogでは引っ掛かっていないみたいですね。

reviewdogはerrorfmtというので結果をパースしていて、F841やN802などのエラーコードのところを (error type)(number)で判断してE, Wのみだしていそうです。

error type (finds a single character):
                            e - error message
                            w - warning message
                            i - info message
                            n - note message

なのでaction-flake8を使わずにaction-setupからxmllintのようにコマンドでやってあげたほうが良さそうかなと今は考えてます。

Tacha-S commented 2 years ago

flake8 config

[flake8]
ignore = F401,W503,D100
max-line-length = 119
import-order-style = pep8
docstring-convention = numpy
copyright-check = True
copyright-regexp = # Copyright \(c\) [0-9]{4} SoftBank Corp\.\n#\n# Licensed under the Apache License, Version 2\.0 \(the "License"\);\n# you may not use this file except in compliance with the License\.\n# You may obtain a copy of the License at\n#\n#     http\:\/\/www\.apache\.org\/licenses\/LICENSE-2\.0\n#\n# Unless required by applicable law or agreed to in writing, software\n# distributed under the License is distributed on an "AS IS" BASIS,\n# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied\.\n# See the License for the specific language governing permissions and\n# limitations under the License\.
select = E,F,W,C,N,D,I

no copyright

#!/usr/bin/env python
# -*- coding: utf-8 -*-
flake8 --config .flake8 test.py
test.py:1:2: C801 Copyright notice not present.

correct copyright

#!/usr/bin/env python
# -*- coding: utf-8 -*-

# Copyright (c) 2021 SoftBank Corp.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
flake8 --config .flake8 test.py

wrong copyright

#!/usr/bin/env python
# -*- coding: utf-8 -*-

# Copyright (c) 2021 SoftBank Corp.
# Author: Sakaguchi
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
flake8 --config .flake8 test.py
test.py:1:2: C801 Copyright notice not present.
Tacha-S commented 2 years ago

とりあえず意見が分かれなさそうな

import-orderはlocalのimport(例えば、cube_python_api内でのcube_python_api.***のimportなど)がcube_python_apiといったパッケージ名を書くとcatkin build時にsite_packagesに登録されてしまうので3rd party扱いになるのがなんとも言えない感じでした。 標準ライブラリと3rd partyで分けてそれぞれを辞書順に並べなおすことはできます。

docstringはまだstyleが決まってないのでそれが決まり次第一定期間は通知のみでCIはfailedしないようにという形でなら取り込めると思います。 ただし、python2ではgoogle styleはバージョンの都合で選択できません。numpyかpep257になります。

flake8-quotesもPEP8でescapeを使うのは避けましょうとありますが、', "はどちらを使ってもいいとあるので、投票にしようかなと思います。

MikhailBertrand commented 2 years ago

flake8-quotesもPEP8でescapeを使うのは避けましょうとありますが、', "はどちらを使ってもいいとあるので、投票にしようかなと思います。

ここ前にも気になって自分も調べたのですが、正直みんな趣味で決めているようなものなので、flake8-quotesは導入しなくても良いかな、と個人的には思います。

Tacha-S commented 2 years ago

確かに、統一されるだけでとくに動作に影響あるとかではないので要らないっていう意見もありですね。

MikhailBertrand commented 2 years ago

完全に余談なのですが、イギリス英語の文章では話し言葉等々をシングルクォーテーションで囲い、アメリカ英語の文章ではダブルクォーテーションで囲うのが基本になっていて、結果としてプログラミングでも結構割れるイメージです。