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

Feature/build linter #43

Merged
merged 8 commits into from Dec 26, 2022
Merged

Feature/build linter #43

merged 8 commits into from Dec 26, 2022

Conversation

w305jack
Copy link
Contributor

@w305jack w305jack commented Dec 22, 2022

Why need this change? / Root cause:

Changes made:

  • update README.md file
  • update requirements.txt file
  • add .pylintrc file
  • add .pre-commit-config.yaml file
  • add linters.yml file under .github/workflows directory

Test Scope / Change impact:

  • CI/CD (linters.yml) should work and check every pull request

issue #17

新增 .pylintrc 設定檔,之後讓 pylint 跟 ci 可以使用

額外 disable messages:
missing-module-docstring
missing-class-docstring
missing-function-docstring
unused-argument
too-many-arguments
too-few-public-methods
too-many-locals
broad-except
bare-except
raise-missing-from

調整項目:
1. 在 requirements.txt 裡,新增 pylint,並指定起始版本為 2.15.5
調整項目:
1. 在 requirements.txt 裡
   - 新增 isort,並指定起始版本為 5.10.1
   - 新增 black,並指定起始版本為 22.10.0
2. 更新 README.md 檔案,加入 isort、black、pylint 的使用方法
加入 .pre-commit-config.yaml 設定檔,使用 hooks 如下
- black
- isort
- pylint

調整項目:
1. 在 requirements.txt 裡,新增 pre-commit,並指定起始版本為 2.20.0
2. 更新 README.md,加入 pre-commit 的使用方法
新增 linters.yml 設定檔,linters workflow 主要針對 3 個相關的 job
- pylint
- isort
- black
調整項目:
1. 新增 3 個 badge 至 README.md
問題:
1. 某正常情境下,exit code 出現非 0

原因:
1. PR 沒有異動到 Python files 時,
   ${{ steps.changed-files.outputs.all_changed_files }} 會為空字串,
   沒有 input 傳入,產生錯誤

調整項目:
1. 執行檢查的地方,寫成條件式,非空字串時才執行
@w305jack w305jack marked this pull request as ready for review December 22, 2022 15:28
@w305jack
Copy link
Contributor Author

這邊打一些補充及想法,根據 #17 (comment) 裡面列的相關 package。

目前 .pylintrc 裡面除了預設的 disable mesaages 之外,另外加了一些,關閉一些無傷大雅的 message。

missing-module-docstring
missing-class-docstring
missing-function-docstring
unused-argument
too-many-arguments
too-few-public-methods
too-many-locals
broad-except
bare-except
raise-missing-from

但整個專案 pylint 下來,大部分 Python 檔案還是都有出現其他的 message,如下:

import-outside-toplevel
cyclic-import
import-self
line-too-long
invalid-name
inconsistent-return-statements
no-else-raise
consider-using-f-string
invalid-envvar-default
unnecessary-pass
trailing-whitespace
lost-exception
unused-variable

以上的錯誤,是比較影響開發的維護性、可讀性跟流暢度,目前 pt_config.pypt_service.py... 等都有出現 message。

目前想法是讓專案 rolling format,在 pre-commit 那一層機制下 (假設 PR 進去了,有 rebase or merge 過,並安裝它),會先在 local 開發端被 block 一次,後續的 commit 應該就會是 formatted 過的了,也剛好讓正在 develop 這幾隻檔案的人,可以順帶調整。

又或者直接開 branch,來特別處理全部 Python 檔案的 code style。

(目前 Action 裡的 workflow - Linters,裡面的 Step - Set changed files,現階段會對發 PR 的 branch,找出其 changed files,並檢查是否符合規範)


後續可以調整的 (開 issue 🤔),如下:

  1. .pylintrc 可以增減項 (讓檢查更嚴格 😈),或者增加 flake8
  2. pre-commit 可以加入其他檔案類型的 hook,像是 shellcheckmarkdownlint-cli
  3. linters.yml (Github Action workflow) 裡的 job。
    • 可以將一些 step 換成外部寫好的 GitHub Action,像是 gh-problem-matcher-wrap (Django 也有使用)、black
    • 對照 pre-commit 加入的 hook 來新增 job。
  4. 增加 mypypyre,來檢查型別。
  5. 開 branch 將剩餘殘檔 format。

@chen-tf
Copy link
Owner

chen-tf commented Dec 24, 2022

後續可以列出會影響開發的 format checking,而比較 minor 的可以考慮放過。
加入 pre-commit 確實可以讓其他 reviewer 更專心在一些重要的檢查上 👍🏽

Comment on lines +19 to +22
pylint~=2.15.5
isort~=5.10.1
black~=22.10.0
pre-commit~=2.20.0
Copy link
Owner

Choose a reason for hiding this comment

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

這邊的 library 也幫忙補充到 Pipfile 中,之後 requirements.txt 會汰換。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

run: |
[[ -n "${{ steps.changed-files.outputs.all_changed_files }}" ]] &&
echo "${{ steps.changed-files.outputs.all_changed_files }}" | xargs black --check --diff ||
echo "0 Python files changed."
Copy link
Owner

Choose a reason for hiding this comment

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

No newline at end of the file. 請幫忙補充最後的換行。
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

entry: pylint
language: system
types: [python]
args: ["-rn", "-sn", "--rcfile=.pylintrc", "--fail-on=I"]
Copy link
Owner

Choose a reason for hiding this comment

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

同上 No newline at end of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

IamSaker
IamSaker previously approved these changes Dec 24, 2022
調整項目:
1. Pipfile 安裝 pylint、isort、black 及 pre-commit
2. linters.yml 加入換行
3. .pre-commit-config.yaml 加入換行
Copy link
Owner

@chen-tf chen-tf left a comment

Choose a reason for hiding this comment

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

LGTM

@chen-tf chen-tf merged commit a23b27e into main Dec 26, 2022
@chen-tf chen-tf deleted the feature/build-linter branch December 26, 2022 16:05
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