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

[CodeStyle] add pre-commit hook remove-tabs for python files #46290

Merged
merged 23 commits into from
Sep 27, 2022

Conversation

SigureMo
Copy link
Member

@SigureMo SigureMo commented Sep 20, 2022

PR types

Others

PR changes

Others

Describe

添加 remove-tabs pre-commit hook,以自动删除 tab,并且移除 Flake8 配置中 ignore 的 W191 和 E101 错误码

本 PR 仅针对 python 相关文件

全局忽略第三方库 gast(#34556 引入),remove-tabs 配置忽略一些单测必须要 tabs 的文件

@SigureMo SigureMo changed the title [CodeStyle] add pre-commit hook remove-tabs for python files [CodeStyle] add pre-commit hook remove-tabs for python files Sep 20, 2022
@paddle-bot-old paddle-bot-old bot added the contributor External developers label Sep 20, 2022
@luotao1 luotao1 self-assigned this Sep 21, 2022
.flake8 Outdated
./build,
# A trick to exclude fluid/ but keep fluid/tests/
./python/paddle/fluid/[!t]**,
./python/paddle/fluid/transpiler/
Copy link
Member Author

@SigureMo SigureMo Sep 21, 2022

Choose a reason for hiding this comment

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

根据 #46288 (comment) 的讨论,目前 Flake8 忽略 python/paddle/fluid 整个目录,但需要单独保留 python/paddle/fluid/tests

Flake8 路径相关使用的是 glob 语法(见 https://en.wikipedia.org/wiki/Glob_(programming) ),glob 语法并不能很好地直接实现这样的需求,Stack Overflow 上也有很多问题说明了这一问题(如 https://stackoverflow.com/questions/20638040/glob-exclude-pattern

这里先使用 ./python/paddle/fluid/[!t]** 以排除所有 ./python/paddle/fluid/ 下第一个字母不是 t 的路径,这样的路径包含了 tests/ 子目录、transpiler/ 子目录、trainer_desc.pytrainer_factory.py,因此实际上其余以 transpiler/trainer_desc.pytrainer_factory.py 也被「豁免」了。为了解决这一问题,使用额外一项排除掉这样的路径,这样通过两条 glob 实现了上述的需求。

E201,E202,E203,E221,E225,E226,E228,E231,E241,E251,E261,E262,E265,E266,E271,E272,E275,
E301,E302,E303,E305,E306,
E401,E402,
E501,E502,
E701,E711,E712,E713,E714,E721,E722,E731,E741,

# F, see https://flake8.pycqa.org/en/latest/user/error-codes.html
F401,F402,F403,F404,F405,
Copy link
Member Author

Choose a reason for hiding this comment

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

E124 和 F404 是原本不存在的问题,统计出错是因为 #46014 (comment) 统计时候包含了 autopep8 产生的增量,因此这里把这俩也删掉了

此前现存问题应该是 68 个,已在 tracking issue 中更新

Copy link
Member Author

@SigureMo SigureMo Sep 25, 2022

Choose a reason for hiding this comment

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

根据最新统计,又移除了 E221、E275、F523、F823,部分应该是由于排除掉了 fluid,另外一部分应该是因为 #46410 移除了部分 disable 掉的格式化,重新格式化后部分错误码消失了

之后在 tracking issue 更新

luotao1
luotao1 previously approved these changes Sep 23, 2022
Copy link
Contributor

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

LGTM

@luotao1 luotao1 merged commit 681d6b1 into PaddlePaddle:develop Sep 27, 2022
@SigureMo SigureMo deleted the tabs/config/py branch September 27, 2022 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants