Skip to content

[CodeStyle][E711] use is/is not for comparison with None - #47452

Merged
luotao1 merged 8 commits into
PaddlePaddle:developfrom
cattidea:E711/fix
Nov 1, 2022
Merged

luotao1 merged 8 commits into
PaddlePaddle:developfrom
cattidea:E711/fix

Conversation

@SigureMo

@SigureMo SigureMo commented Oct 28, 2022 •

Copy link
Copy Markdown
Collaborator

PR types

Others

PR changes

Others

Describe

解决 Flake8 E711 问题(comparison to None should be ‘if cond is None:’)

  • x == None -> x is None
  • x != None -> x is not None

并调整单测中的相关使用(与 None 相关的比较),如 self.assertTrue(x is None) -> self.assertIsNone(x),可以优化报错信息:

import unittest

class Test1(unittest.TestCase):
    def test(self):
        x = 1
        self.assertTrue(x is None)

# 不会说明 x 到底是什么值
Test1().test() # AssertionError: False is not true

class Test2(unittest.TestCase):
    def test(self):
        x = 1
        self.assertIsNone(x)

# 会同时说明 x 是什么
Test2().test() # AssertionError: 1 is not None

全部替换如下:

  • self.assertTrue(x is None) -> self.assertIsNone(x)
  • self.assertTrue(x is not None) -> self.assertIsNotNone(x)
  • self.assertFalse(x is None) -> self.assertIsNotNone(x)
  • self.assertFalse(x is not None) -> self.assertIsNone(x)(无此情况)
  • self.assertEqual(x, None) -> self.assertIsNone(x)
  • self.assertNotEqual(x, None) -> self.assertIsNotNone(x)

具体所使用工具及命令如下:

# ast-grep, docs: https://ast-grep.github.io/
# clone ast-grep to local
git clone https://github.com/ast-grep/ast-grep.git
cd ast-grep
# run ast-grep in dev mode
cargo run -- --pattern '$COND == None' --rewrite '$COND is None' --lang py -i /path/to/paddle
cargo run -- --pattern '$COND != None' --rewrite '$COND is not None' --lang py -i /path/to/paddle
# ...

Related links

@paddle-bot

paddle-bot Bot commented Oct 28, 2022

Copy link
Copy Markdown

你的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 Oct 28, 2022
@SigureMo SigureMo changed the title [WIP][CodeStyle][E711] use is/is not for comparison with None [CodeStyle][E711] use is/is not for comparison with None Oct 28, 2022
@luotao1 luotao1 self-assigned this Oct 31, 2022
@luotao1

luotao1 commented Oct 31, 2022

Copy link
Copy Markdown
Collaborator

ast-grep, docs: https://ast-grep.github.io/

是否考虑将修复工具引入precommit配置?

@SigureMo

SigureMo commented Oct 31, 2022 •

Copy link
Copy Markdown
Collaborator Author

是否考虑将修复工具引入precommit配置?

ast-grep 的话暂时不考虑,一方面它并不是 python 生态的,而是 Rust / Node.js 生态的工具,目前不方便集成,另一方面它刚刚开发几个月,非常不成熟

luotao1
luotao1 previously approved these changes Nov 1, 2022
'dim': axis if axis is not None and axis != [] and axis != () else [0],
'keep_dim': keepdim,
'reduce_all': reduce_all_flag,
}

@luotao1 luotao1 Nov 1, 2022 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

2022-10-31 18:16:21 0. APIs without core.ops: 
2022-10-31 18:16:21 paddle.fluid.layers.nn.reduce_any
2022-10-31 18:16:21 You must have one RD (JiabinYang (Recommend) or wanghuancoder, phlrain) approval for the api change for the opreator-related api without '_C_ops'.
2022-10-31 18:16:21 For more details, please click [https://github.com/PaddlePaddle/Paddle/wiki/paddle_api_development_manual.md]

请 @wanghuancoder @JiabinYang review

@luotao1

luotao1 commented Nov 1, 2022

Copy link
Copy Markdown
Collaborator

ast-grep 的话暂时不考虑,一方面它并不是 python 生态的,而是 Rust / Node.js 生态的工具,目前不方便集成,另一方面它刚刚开发几个月,非常不成熟

Got it. 因为本PR 【E711】和 【E712】#47464 的修改不能使用自动化,可以将两个描述中的修改方法写在wiki里,在增量拦截的时候 #47465 提示给开发者。

@SigureMo

SigureMo commented Nov 1, 2022 •

Copy link
Copy Markdown
Collaborator Author

因为本PR 【E711】和 【E712】#47464 的修改不能使用自动化,可以将两个描述中的修改方法写在wiki里,在增量拦截的时候 #47465 提示给开发者。

是在 CI 使用 grep 匹配增量中的 x == None 这类 E711、E712 问题吗,不过按照正常流程的话(安装了 pre-commit),有这类问题的 commit 是无法通过 pre-commit 的,所以这样的问题不会到 CI 这一步,也就无法展示提示信息,除非开发者没有安装 pre-commit

另外就是 grep 无法通过 ast 分析代码,可能会造成一些误检

@luotao1

luotao1 commented Nov 1, 2022

Copy link
Copy Markdown
Collaborator

有这类问题的 commit 是无法通过 pre-commit 的,所以这样的问题不会到 CI 这一步,也就无法展示提示信息,除非开发者没有安装 pre-commit

确实如此。后续可以在 代码风格检查指南 里加一下常见错误码(无法自动格式化) 的处理方式。

@luotao1
luotao1 requested a review from wanghuancoder November 1, 2022 07:35
wanghuancoder
wanghuancoder previously approved these changes Nov 1, 2022

@wanghuancoder wanghuancoder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@luotao1

luotao1 commented Nov 1, 2022

Copy link
Copy Markdown
Collaborator

请解决下冲突

@SigureMo
SigureMo dismissed stale reviews from wanghuancoder and luotao1 via fb39d22 November 1, 2022 09:21
@SigureMo

SigureMo commented Nov 1, 2022

Copy link
Copy Markdown
Collaborator Author

请解决下冲突

已解决~

@luotao1
luotao1 merged commit a35a4a5 into PaddlePaddle:develop Nov 1, 2022
@SigureMo
SigureMo deleted the E711/fix branch November 1, 2022 14:15
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.

[E711] comparison to None should be ‘if cond is None:’

3 participants