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][B005] Using .strip() with multi-character strings is misleading the reader #52103

Closed
wants to merge 2 commits into from

Conversation

gouzil
Copy link
Member

@gouzil gouzil commented Mar 24, 2023

PR types

Others

PR changes

Others

Describe

str.strip("demo")替换成str.strip("demo", '')

官方建议为将strip替换为 removeprefixremovesuffix 但是这两个方法仅存在于3.9以后的版本 3.9更新公告

原文:

B005: Using .strip() with multi-character strings is misleading the reader. It looks like stripping a substring. Move your character set to a constant if this is deliberate. Use .replace(), .removeprefix(), .removesuffix() or regular expressions to remove string fragments.

是否可以引入本 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.

@SigureMo
Copy link
Member

str.strip("demo") 替换成 str.strip("demo", '')

是怎么得到这个结论的呢,根据 strip 文档(Built-in Types),strip 没有第二个参数,这样根本跑不通

该 rule 确实发现了目前的很多潜藏的 bug,但就目前 Ruff 的检测规则和检测效果来看,是有误检的风险的,因为任何 object 都可能有 strip 方法,Ruff 不会判断是否是 str(当然这也很难)

因此本 Rule 并不适合引入,但可以修复下当前检测出来的部分问题

@gouzil
Copy link
Member Author

gouzil commented Mar 24, 2023

将 替换成str.strip("demo")``str.strip("demo", '')

是怎么得到这个结论的呢,根据 strip 文档(Built-in Types),strip 没有第二个参数,这样根本跑不通

该 rule 确实发现了目前的很多潜藏的 bug,但就目前 Ruff 的检测规则和检测效果来看,是有误检的风险的,因为任何 object 都可能有 strip 方法,Ruff 不会判断是否是 str(当然这也很难)

因此本 Rule 并不适合引入,但可以修复下当前检测出来的部分问题

不好意思看成 replace 了 , 您说的 "当前检测出来的部分问题" 是指tools/infrt/get_compat_kernel_signature.py这个吗

@SigureMo
Copy link
Member

不好意思看成 replace 了 , 您说的 "当前检测出来的部分问题" 是指tools/infrt/get_compat_kernel_signature.py 这个吗

大多数是有问题的吧,其实也可以暂时不修改,等 3.9 再引入这条 rule,因为目前并没有太优雅的修复方式,而且可能对运行效果有影响

@gouzil
Copy link
Member Author

gouzil commented Mar 24, 2023

不好意思看成 replace 了 , 您说的 “当前检测出来的部分问题” 是指 这个吗tools/infrt/get_compat_kernel_signature.py

大多数是有问题的吧,其实也可以暂时不修改,等 3.9 再引入这条 rule,因为目前并没有太优雅的修复方式,而且可能对运行效果有影响

那就暂时先不修改了吧,等到不支持老版本在做修改

@paddle-bot
Copy link

paddle-bot bot commented Mar 24, 2023

很抱歉,经过我们的反复讨论,你的PR暂未达到合入标准,请阅读飞桨原生算子开发规范,你可以重新提交新的PR,我们先将此PR关闭,感谢你的贡献。
Sorry to inform you that through our discussion, your PR fails to meet the merging standard (Reference: Paddle Custom Operator Design Doc). You can also submit an new one. Thank you.

@gouzil gouzil deleted the codeStyle-b005 branch March 24, 2023 13:06
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