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.

Sign up for GitHub

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

Jump to bottom

[CodeStyle][PLC3002][PLE1205] simplify lambda and add missing placeholder to logger template #52133

Merged
merged 6 commits into from
Mar 27, 2023
Merged

[CodeStyle][PLC3002][PLE1205] simplify lambda and add missing placeholder to logger template #52133

merged 6 commits into from
Mar 27, 2023

Conversation

gouzil
Copy link
Member

@gouzil gouzil commented Mar 24, 2023 •
edited
Loading

PR types

Others

PR changes

Others

Describe

PLC3002

参考文档pylint unnecessary-direct-lambda-call / C3002

对 Lambda 表达式进行简化

-    out = add_func(y) if y or y < 2 else (lambda x: x**2)(y)
+    out = add_func(y) if y or y < 2 else y**2

PLE1205

参考文档pylint logging-too-many-args / E1205

对日志输出优化

-     logger.debug("not message:", name)
+    logger.debug("not message: %s", name)

是否可以引入本 rule：✅ 如上所述，可以引入
是否可引入自动修复：❌手动修复

Related links

@paddle-bot
Copy link

paddle-bot bot commented Mar 24, 2023

你的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 Mar 24, 2023
@gouzil gouzil mentioned this pull request Mar 24, 2023
Closed
@gouzil gouzil changed the title [CodeStyle][PLC3002] simplify Lambda [CodeStyle][PLC3002][PLE1205] simplify Lambda Mar 24, 2023
@SigureMo
Copy link
Member

参考文档pylint unnecessary-direct-lambda-call / C3002

emmm，你可能理解有误，我要的官方文档是指 Python 对于这些语法、函数的文档（包括 PEP 这种较为权威的文档，最差也得是 StackOverflow 这种高质量的讨论），Linter 本身（Ruff 也好，Ruff 所实现的 PyLint 原规则也好）都认为自己做的有道理，这些文档是没有太大意义的，这里的调研主要是指你对这个改动是如何理解的，是如何去评价 Linter 本身的，引用 Linter 本身的文档并不能得到多少有用的信息

SigureMo
SigureMo reviewed Mar 24, 2023
View reviewed changes
python/paddle/fluid/tests/unittests/dygraph_to_static/test_lambda.py Outdated
@@ -37,8 +37,8 @@ def call_lambda_as_func(x):
def call_lambda_directly(x):
x = fluid.dygraph.to_variable(x)

y = (lambda x, y: x + y)(x, x)
out = (lambda x: paddle.mean(x))(y)
Copy link
Member

Choose a reason for hiding this comment

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

根据文件名和动转静的特殊性，这里明显就是为了测试 lambda 语法是否能够正确转写的，因此不可以修改，该 rule 不适合引入

Copy link
Member Author

Choose a reason for hiding this comment

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

以下是一些关于 lambdas 的讨论:
https://stackoverflow.com/questions/890128/how-are-lambdas-useful/890188

https://stackoverflow.com/questions/20026382/python-callback-vs-node-callback-point-to-point-comparisons/20026692#20026692

我的看法是，像(lambda x: x**2)(y)这种完全可以简化, 使用 lambda 会让阅读变得复杂

所以个人建议是使用[tool.ruff.per-file-ignores]排除当前文件, 来应用该 rule

Copy link
Member

Choose a reason for hiding this comment

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

所以个人建议是使用[tool.ruff.per-file-ignores]排除当前文件, 来应用该 rule

可以

python/paddle/distributed/fleet/base/distributed_strategy.py
@@ -543,7 +543,7 @@ def set_table_config(msg, config_name, configs, index=0):
getattr(msg, field.name), name, configs
)
else:
logger.debug("not message:", name)
Copy link
Member

Choose a reason for hiding this comment

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

该 rule 再多调研一下吧，如果调研结果合适可以引入

Copy link
Member Author

Choose a reason for hiding this comment

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

https://docs.python.org/3/howto/logging.html#optimization

Copy link
Member

Choose a reason for hiding this comment

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

对日志输出优化

https://docs.python.org/3/howto/logging.html#optimization

这里并不是优化，而是避免出现错误的使用方式，目前代码里的使用方式是错误的

image

应引用文档 https://docs.python.org/3/howto/logging.html#logging-variable-data ，参数数量 > 占位符数量，会触发运行时错误

本 rule 可引入，可避免 bug
本 rule 无自动修复功能，需要开发者手动修复

SigureMo
SigureMo previously approved these changes Mar 25, 2023
View reviewed changes
Copy link
Member

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

LGTM

@SigureMo SigureMo changed the title [CodeStyle][PLC3002][PLE1205] simplify Lambda [CodeStyle][PLC3002][PLE1205] simplify lambda and add missing placeholder to logger template Mar 25, 2023
@luotao1
Copy link
Contributor

luotao1 commented Mar 25, 2023

辛苦解决下冲突

@gouzil
Copy link
Member Author

gouzil commented Mar 25, 2023

我好像点到了什么，怎么被 dismissed 了

@SigureMo
Copy link
Member

我好像点到了什么，怎么被 dismissed 了

Paddle 的设置就是新的非空 commit 会取消已有 approval，这个没关系的

SigureMo
SigureMo reviewed Mar 25, 2023
View reviewed changes
Copy link
Member

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

LGTM

pyproject.toml Outdated Show resolved Hide resolved
SigureMo
SigureMo previously approved these changes Mar 25, 2023
View reviewed changes
Copy link
Member

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

LGTM

@SigureMo SigureMo mentioned this pull request Mar 25, 2023
Merged
@luotao1
Copy link
Contributor

luotao1 commented Mar 25, 2023

辛苦解决下冲突

@gouzil
Copy link
Member Author

gouzil commented Mar 25, 2023

辛苦解决下冲突

Done

SigureMo
SigureMo approved these changes Mar 27, 2023
View reviewed changes
Copy link
Member

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

LGTM

@SigureMo SigureMo merged commit b166581 into PaddlePaddle:develop Mar 27, 2023
@gouzil gouzil deleted the codeStyle-PLC3002 branch May 15, 2023 14:31
@gouzil gouzil mentioned this pull request Oct 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SigureMo SigureMo SigureMo approved these changes

Assignees

@luotao1 luotao1

@SigureMo SigureMo

Labels
contributor External developers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gouzil @SigureMo @luotao1