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][isort] introducing isort (part1) #46475

Merged
merged 3 commits into from
Nov 25, 2022

Conversation

SigureMo
Copy link
Member

@SigureMo SigureMo commented Sep 25, 2022

PR types

Others

PR changes

Others

Describe

RFC: Python 引入 import 区域自动重排工具 isort 方案

引入工具 isort 以及对部分文件进行格式化

isort 是一个对文件中 imports 部分进行重排的工具,会依照 PEP 8 - imports 规范以下规则对 imports 进行重排

Imports should be grouped in the following order:

  1. Standard library imports.
  2. Related third party imports.
  3. Local application/library specific imports.

可以极大改善 imports 部分的代码风格,使得开发人员更容易了解模块之间的依赖关系,且能够自动去除 imports 部分重复的导入

isort 同时可以稍微消除一部分 flake8 错误码1,如完全消除 E401(multiple imports on one line)(存量 9 -> 0),因为可以去重所以可以消除部分 F811(redefinition of unused name from line N)(存量 123 -> 46),但这只是引入 isort 额外的收获,其主要作用还是改善 imports 部分的代码风格(虽然这不在 flake8 错误码里,但是 PEP 8 中描述的)

其他社区使用 isort 的例子:

实施步骤:

  • pre-commit 配置添加:
-   repo: https://github.com/pycqa/isort
    rev: 5.10.1
    hooks:
    -   id: isort
  • pyproject.toml 配置添加
[tool.isort]
profile = "black"
line_length = 80
known_first_party = ["paddle"]
skip = ["build", "__init__.py"]
extend_skip_glob = [
    # These files do not need to be formatted,
    # see .flake8 for more details
    "python/paddle/fluid/[!t]**",
    "python/paddle/fluid/tra**",
    "*_pb2.py",
    "python/paddle/utils/gast/**",
    "python/paddle/fluid/tests/unittests/npu/**",
    "python/paddle/fluid/tests/unittests/mlu/**",

    # These files will be fixed in the future
    "cmake/**",
    "paddle/**",
    "r/**",
    "tools/**",
    "python/paddle/[!f]**",
    "python/paddle/fluid/tests/unittests/[t-z]**",
    "python/paddle/fluid/tests/unittests/dygraph_to_static/test_error.py",
    "python/paddle/fluid/tests/unittests/dygraph_to_static/**",
    "python/paddle/fluid/tests/unittests/ipu/test_dy2static_ipu.py",
]

类似 flake8 的 F401 引入,先在配置中 ignore 一部分,但由于 isort 只是对 import 排序,大多数情况不会有影响(少数的比如一个 import 中修改全局状态,后一个 import 依赖于此全局状态可能会有影响),因此可相对于 F401 目录的划分粒度大一些(利用 10 个以内 PR 完成,实际应该会更少些)

  • 「全量」格式化
isort .

由于配置里 ignore 了部分路径,因此只会格式化一部分路径,之后每个 PR 解开一部分路径之后 isort . 即可,针对有问题的先 ignore 掉在之后 PR 里解决

Note 分 PR 的原因

  1. 避免频繁冲突
  2. 避免像 black 引入时需要临时修改 PR-CI-Coverage 流水线
  3. 由于仅仅格式化 imports 部分,不会像 black 那样造成很大的影响,不需要专门找时间来 merge

Footnotes

  1. [CodeStyle] Flake8 tracking issue #46039

@paddle-bot
Copy link

paddle-bot bot commented Sep 25, 2022

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot paddle-bot bot added contributor External developers status: proposed labels Sep 25, 2022
@SigureMo SigureMo changed the title [CodeStyle][E401] use isort sort imports [Don't merge][CodeStyle][E401] use isort sort imports Sep 25, 2022
@SigureMo

This comment was marked as outdated.

@luotao1 luotao1 self-assigned this Sep 27, 2022
@SigureMo SigureMo closed this Sep 28, 2022
@SigureMo SigureMo reopened this Sep 28, 2022
@SigureMo SigureMo changed the title [Don't merge][CodeStyle][E401] use isort sort imports [Don't merge][CodeStyle] use isort sort imports Oct 24, 2022
@SigureMo SigureMo changed the title [Don't merge][CodeStyle] use isort sort imports [Don't merge][CodeStyle] introducing isort and using it to sort imports Oct 27, 2022
@SigureMo SigureMo changed the title [Don't merge][CodeStyle] introducing isort and using it to sort imports [CodeStyle] introducing isort and using it to sort imports (part1) Oct 28, 2022
luotao1
luotao1 previously approved these changes Oct 28, 2022
@SigureMo SigureMo changed the title [CodeStyle] introducing isort and using it to sort imports (part1) [CodeStyle][isort] introducing isort and using it to sort imports (part1) Oct 28, 2022
raindrops2sea
raindrops2sea previously approved these changes Nov 1, 2022
@luotao1
Copy link
Contributor

luotao1 commented Nov 24, 2022

请解决下冲突

@SigureMo SigureMo force-pushed the isort/fix/all branch 2 times, most recently from 109ae95 to 0d6840a Compare November 24, 2022 06:15
@SigureMo SigureMo changed the title [CodeStyle][isort] introducing isort and using it to sort imports (part1) [CodeStyle][isort] introducing isort (part1) Nov 24, 2022
@SigureMo
Copy link
Member Author

@luotao1 现在已经没有冲突了,是否可以准备合入这个 PR 了?

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.

3 participants