From da6bc2db2b963ce9deac12de96621fe9df187968 Mon Sep 17 00:00:00 2001 From: theirix Date: Sun, 8 Oct 2023 11:05:41 +0300 Subject: [PATCH 01/10] Skip consider-using-join for non-empty separators Only suggest joining with separators when a new option 'suggest-join-with-non-empty-separator' is specified. --- .../refactoring/refactoring_checker.py | 25 +++++++++++++++++-- tests/functional/c/consider/consider_join.py | 8 +++--- tests/functional/c/consider/consider_join.txt | 4 --- tests/functional/c/consider/consider_join2.py | 24 ++++++++++++++++++ tests/functional/c/consider/consider_join2.rc | 2 ++ .../functional/c/consider/consider_join2.txt | 4 +++ 6 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 tests/functional/c/consider/consider_join2.py create mode 100644 tests/functional/c/consider/consider_join2.rc create mode 100644 tests/functional/c/consider/consider_join2.txt diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index d29326693a..fc4ca7ed9a 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -506,6 +506,18 @@ class RefactoringChecker(checkers.BaseTokenChecker): "and no message will be printed.", }, ), + ( + "suggest-join-with-non-empty-separator", + { + "default": False, + "type": "yn", + "metavar": "", + "help": ( + "Suggest using join for consider-using-join " + "when a non-empty item separator is used." + ), + }, + ), ) def __init__(self, linter: PyLinter) -> None: @@ -514,6 +526,7 @@ def __init__(self, linter: PyLinter) -> None: self._consider_using_with_stack = ConsiderUsingWithStack() self._init() self._never_returning_functions: set[str] = set() + self._suggest_join_with_non_empty_separator: bool = False def _init(self) -> None: self._nested_blocks: list[NodesWithNestedBlocks] = [] @@ -527,6 +540,9 @@ def open(self) -> None: self._never_returning_functions = set( self.linter.config.never_returning_functions ) + self._suggest_join_with_non_empty_separator = ( + self.linter.config.suggest_join_with_non_empty_separator + ) @cached_property def _dummy_rgx(self) -> Pattern[str]: @@ -1667,8 +1683,7 @@ def _dict_literal_suggestion(node: nodes.Call) -> str: suggestion = ", ".join(elements) return f"{{{suggestion}{', ... ' if len(suggestion) > 64 else ''}}}" - @staticmethod - def _name_to_concatenate(node: nodes.NodeNG) -> str | None: + def _name_to_concatenate(self, node: nodes.NodeNG) -> str | None: """Try to extract the name used in a concatenation loop.""" if isinstance(node, nodes.Name): return cast("str | None", node.name) @@ -1680,6 +1695,12 @@ def _name_to_concatenate(node: nodes.NodeNG) -> str | None: ] if len(values) != 1 or not isinstance(values[0].value, nodes.Name): return None + # If there are more values in joined string than formatted values, + # they are probably separators. + # Allow them only if the option `suggest-join-with-non-empty-separator` is set + with_separators = len(node.values) > len(values) + if with_separators and not self._suggest_join_with_non_empty_separator: + return None return cast("str | None", values[0].value.name) def _check_consider_using_join(self, aug_assign: nodes.AugAssign) -> None: diff --git a/tests/functional/c/consider/consider_join.py b/tests/functional/c/consider/consider_join.py index 9f785b64d9..68ff779dbb 100644 --- a/tests/functional/c/consider/consider_join.py +++ b/tests/functional/c/consider/consider_join.py @@ -19,19 +19,19 @@ result = 'a' for number in ['1', '2', '3']: - result += f'b{number}' # [consider-using-join] + result += f'b{number}' assert result == 'ab1b2b3' assert result == 'b'.join(['a', '1', '2', '3']) result = 'a' for number in ['1', '2', '3']: - result += f'{number}c' # [consider-using-join] + result += f'{number}c' assert result == 'a1c2c3c' assert result == 'a' + 'c'.join(['1', '2', '3']) + 'c' result = 'a' for number in ['1', '2', '3']: - result += f'b{number}c' # [consider-using-join] + result += f'b{number}c' assert result == 'ab1cb2cb3c' assert result == 'ab' + 'cb'.join(['1', '2', '3']) + 'c' @@ -41,7 +41,7 @@ result = '' for number in ['1', '2', '3']: - result += f"{number}, " # [consider-using-join] + result += f"{number}, " result = result[:-2] result = 0 # result is not a string diff --git a/tests/functional/c/consider/consider_join.txt b/tests/functional/c/consider/consider_join.txt index fa9b427e38..9ead130089 100644 --- a/tests/functional/c/consider/consider_join.txt +++ b/tests/functional/c/consider/consider_join.txt @@ -2,11 +2,7 @@ consider-using-join:6:4:6:20::Consider using str.join(sequence) for concatenatin consider-using-join:10:4:10:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:14:4:14:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:18:4:18:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED -consider-using-join:22:4:22:26::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED -consider-using-join:28:4:28:26::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED -consider-using-join:34:4:34:27::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:40:4:40:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED -consider-using-join:44:4:44:27::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:85:4:85:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:89:4:89:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:93:4:93:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED diff --git a/tests/functional/c/consider/consider_join2.py b/tests/functional/c/consider/consider_join2.py new file mode 100644 index 0000000000..0795ce523f --- /dev/null +++ b/tests/functional/c/consider/consider_join2.py @@ -0,0 +1,24 @@ +# pylint: disable=missing-docstring,invalid-name,undefined-variable,multiple-statements + +result = 'a' +for number in ['1', '2', '3']: + result += f'b{number}' # [consider-using-join] +assert result == 'ab1b2b3' +assert result == 'b'.join(['a', '1', '2', '3']) + +result = 'a' +for number in ['1', '2', '3']: + result += f'{number}c' # [consider-using-join] +assert result == 'a1c2c3c' +assert result == 'a' + 'c'.join(['1', '2', '3']) + 'c' + +result = 'a' +for number in ['1', '2', '3']: + result += f'b{number}c' # [consider-using-join] +assert result == 'ab1cb2cb3c' +assert result == 'ab' + 'cb'.join(['1', '2', '3']) + 'c' + +result = '' +for number in ['1', '2', '3']: + result += f"{number}, " # [consider-using-join] +result = result[:-2] diff --git a/tests/functional/c/consider/consider_join2.rc b/tests/functional/c/consider/consider_join2.rc new file mode 100644 index 0000000000..3c53da1efe --- /dev/null +++ b/tests/functional/c/consider/consider_join2.rc @@ -0,0 +1,2 @@ +[variables] +suggest-join-with-non-empty-separator=yes diff --git a/tests/functional/c/consider/consider_join2.txt b/tests/functional/c/consider/consider_join2.txt new file mode 100644 index 0000000000..53808b2ac3 --- /dev/null +++ b/tests/functional/c/consider/consider_join2.txt @@ -0,0 +1,4 @@ +consider-using-join:5:4:5:26::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED +consider-using-join:11:4:11:26::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED +consider-using-join:17:4:17:27::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED +consider-using-join:23:4:23:27::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED From ebdd4081d8ad03f196b265040dc36230c2e12af9 Mon Sep 17 00:00:00 2001 From: theirix Date: Sun, 8 Oct 2023 11:14:50 +0300 Subject: [PATCH 02/10] Add news fragments --- doc/whatsnew/fragments/8701.feature | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 doc/whatsnew/fragments/8701.feature diff --git a/doc/whatsnew/fragments/8701.feature b/doc/whatsnew/fragments/8701.feature new file mode 100644 index 0000000000..4bb7f6406f --- /dev/null +++ b/doc/whatsnew/fragments/8701.feature @@ -0,0 +1,3 @@ +Skip ``consider-using-join`` check for non-empty separators unless a ``suggest-join-with-non-empty-separator`` option is set to ``yes``. + +Closes #8701 From de43e1fc317976efad0e3d0f12fc3837db018966 Mon Sep 17 00:00:00 2001 From: theirix Date: Sun, 8 Oct 2023 11:36:44 +0300 Subject: [PATCH 03/10] Regenerated option documentation --- doc/user_guide/configuration/all-options.rst | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/doc/user_guide/configuration/all-options.rst b/doc/user_guide/configuration/all-options.rst index 178ee0b195..43b6e0e26e 100644 --- a/doc/user_guide/configuration/all-options.rst +++ b/doc/user_guide/configuration/all-options.rst @@ -1196,6 +1196,13 @@ Standard Checkers **Default:** ``('sys.exit', 'argparse.parse_error')`` +--suggest-join-with-non-empty-separator +""""""""""""""""""""""""""""""""""""""" +*Suggest using join for consider-using-join when a non-empty item separator is used.* + +**Default:** ``False`` + + .. raw:: html @@ -1211,6 +1218,8 @@ Standard Checkers never-returning-functions = ["sys.exit", "argparse.parse_error"] + suggest-join-with-non-empty-separator = false + .. raw:: html From 05c78faa6ccfc5259686c3bfb9bab0227d78f55f Mon Sep 17 00:00:00 2001 From: theirix Date: Sun, 8 Oct 2023 14:23:15 +0300 Subject: [PATCH 04/10] Improve option description Co-authored-by: Pierre Sassoulas --- pylint/checkers/refactoring/refactoring_checker.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index fc4ca7ed9a..478efb809a 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -513,8 +513,9 @@ class RefactoringChecker(checkers.BaseTokenChecker): "type": "yn", "metavar": "", "help": ( - "Suggest using join for consider-using-join " - "when a non-empty item separator is used." + "Let 'consider-using-join' be raised when the separator to " + "join on would be non-empty (resulting in expected fixes " + "of the type: ``"- " + "\n- ".join(items)``)" ), }, ), From 3b1c1435d6dee790cf68acea0157e63f86148ab9 Mon Sep 17 00:00:00 2001 From: theirix Date: Sun, 8 Oct 2023 14:26:11 +0300 Subject: [PATCH 05/10] Rename test cases to consider_join_for_non_empty_separator --- ...consider_join2.py => consider_join_for_non_empty_separator.py} | 0 ...consider_join2.rc => consider_join_for_non_empty_separator.rc} | 0 ...nsider_join2.txt => consider_join_for_non_empty_separator.txt} | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename tests/functional/c/consider/{consider_join2.py => consider_join_for_non_empty_separator.py} (100%) rename tests/functional/c/consider/{consider_join2.rc => consider_join_for_non_empty_separator.rc} (100%) rename tests/functional/c/consider/{consider_join2.txt => consider_join_for_non_empty_separator.txt} (100%) diff --git a/tests/functional/c/consider/consider_join2.py b/tests/functional/c/consider/consider_join_for_non_empty_separator.py similarity index 100% rename from tests/functional/c/consider/consider_join2.py rename to tests/functional/c/consider/consider_join_for_non_empty_separator.py diff --git a/tests/functional/c/consider/consider_join2.rc b/tests/functional/c/consider/consider_join_for_non_empty_separator.rc similarity index 100% rename from tests/functional/c/consider/consider_join2.rc rename to tests/functional/c/consider/consider_join_for_non_empty_separator.rc diff --git a/tests/functional/c/consider/consider_join2.txt b/tests/functional/c/consider/consider_join_for_non_empty_separator.txt similarity index 100% rename from tests/functional/c/consider/consider_join2.txt rename to tests/functional/c/consider/consider_join_for_non_empty_separator.txt From 1b65b7e0f07f913be33c5fac61f1f6bfabf3bb03 Mon Sep 17 00:00:00 2001 From: theirix Date: Sun, 8 Oct 2023 14:27:34 +0300 Subject: [PATCH 06/10] Fix option description --- pylint/checkers/refactoring/refactoring_checker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 478efb809a..2fb15bc8db 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -515,7 +515,7 @@ class RefactoringChecker(checkers.BaseTokenChecker): "help": ( "Let 'consider-using-join' be raised when the separator to " "join on would be non-empty (resulting in expected fixes " - "of the type: ``"- " + "\n- ".join(items)``)" + 'of the type: ``"- " + "\n- ".join(items)``)' ), }, ), From f91ff78f2aa782ed0c858df2f11457bdc8fde5a1 Mon Sep 17 00:00:00 2001 From: theirix Date: Sun, 8 Oct 2023 16:25:22 +0300 Subject: [PATCH 07/10] Regenerate option docs --- doc/user_guide/configuration/all-options.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/user_guide/configuration/all-options.rst b/doc/user_guide/configuration/all-options.rst index 43b6e0e26e..68b4b7515d 100644 --- a/doc/user_guide/configuration/all-options.rst +++ b/doc/user_guide/configuration/all-options.rst @@ -1198,7 +1198,8 @@ Standard Checkers --suggest-join-with-non-empty-separator """"""""""""""""""""""""""""""""""""""" -*Suggest using join for consider-using-join when a non-empty item separator is used.* +*Let 'consider-using-join' be raised when the separator to join on would be non-empty (resulting in expected fixes of the type: ``"- " + " +- ".join(items)``)* **Default:** ``False`` From 8903a16f0808f5c5af2d1caad40ea9a4ceee7902 Mon Sep 17 00:00:00 2001 From: theirix Date: Mon, 9 Oct 2023 00:19:08 +0300 Subject: [PATCH 08/10] Option suggest-join-with-non-empty-separator defaults to true Co-authored-by: Pierre Sassoulas --- pylint/checkers/refactoring/refactoring_checker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 2fb15bc8db..3b074a7c02 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -509,7 +509,7 @@ class RefactoringChecker(checkers.BaseTokenChecker): ( "suggest-join-with-non-empty-separator", { - "default": False, + "default": True, "type": "yn", "metavar": "", "help": ( From db882dc478b83e484e113bd3711d21c02c98dd3d Mon Sep 17 00:00:00 2001 From: theirix Date: Mon, 9 Oct 2023 12:48:01 +0300 Subject: [PATCH 09/10] Updates test cases for option defaulted to true --- tests/functional/c/consider/consider_join.py | 8 ++++---- tests/functional/c/consider/consider_join.txt | 4 ++++ .../c/consider/consider_join_for_non_empty_separator.py | 8 ++++---- .../c/consider/consider_join_for_non_empty_separator.rc | 2 +- .../c/consider/consider_join_for_non_empty_separator.txt | 4 ---- 5 files changed, 13 insertions(+), 13 deletions(-) delete mode 100644 tests/functional/c/consider/consider_join_for_non_empty_separator.txt diff --git a/tests/functional/c/consider/consider_join.py b/tests/functional/c/consider/consider_join.py index 68ff779dbb..9f785b64d9 100644 --- a/tests/functional/c/consider/consider_join.py +++ b/tests/functional/c/consider/consider_join.py @@ -19,19 +19,19 @@ result = 'a' for number in ['1', '2', '3']: - result += f'b{number}' + result += f'b{number}' # [consider-using-join] assert result == 'ab1b2b3' assert result == 'b'.join(['a', '1', '2', '3']) result = 'a' for number in ['1', '2', '3']: - result += f'{number}c' + result += f'{number}c' # [consider-using-join] assert result == 'a1c2c3c' assert result == 'a' + 'c'.join(['1', '2', '3']) + 'c' result = 'a' for number in ['1', '2', '3']: - result += f'b{number}c' + result += f'b{number}c' # [consider-using-join] assert result == 'ab1cb2cb3c' assert result == 'ab' + 'cb'.join(['1', '2', '3']) + 'c' @@ -41,7 +41,7 @@ result = '' for number in ['1', '2', '3']: - result += f"{number}, " + result += f"{number}, " # [consider-using-join] result = result[:-2] result = 0 # result is not a string diff --git a/tests/functional/c/consider/consider_join.txt b/tests/functional/c/consider/consider_join.txt index 9ead130089..fa9b427e38 100644 --- a/tests/functional/c/consider/consider_join.txt +++ b/tests/functional/c/consider/consider_join.txt @@ -2,7 +2,11 @@ consider-using-join:6:4:6:20::Consider using str.join(sequence) for concatenatin consider-using-join:10:4:10:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:14:4:14:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:18:4:18:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED +consider-using-join:22:4:22:26::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED +consider-using-join:28:4:28:26::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED +consider-using-join:34:4:34:27::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:40:4:40:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED +consider-using-join:44:4:44:27::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:85:4:85:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:89:4:89:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED consider-using-join:93:4:93:20::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED diff --git a/tests/functional/c/consider/consider_join_for_non_empty_separator.py b/tests/functional/c/consider/consider_join_for_non_empty_separator.py index 0795ce523f..16a5f370c7 100644 --- a/tests/functional/c/consider/consider_join_for_non_empty_separator.py +++ b/tests/functional/c/consider/consider_join_for_non_empty_separator.py @@ -2,23 +2,23 @@ result = 'a' for number in ['1', '2', '3']: - result += f'b{number}' # [consider-using-join] + result += f'b{number}' assert result == 'ab1b2b3' assert result == 'b'.join(['a', '1', '2', '3']) result = 'a' for number in ['1', '2', '3']: - result += f'{number}c' # [consider-using-join] + result += f'{number}c' assert result == 'a1c2c3c' assert result == 'a' + 'c'.join(['1', '2', '3']) + 'c' result = 'a' for number in ['1', '2', '3']: - result += f'b{number}c' # [consider-using-join] + result += f'b{number}c' assert result == 'ab1cb2cb3c' assert result == 'ab' + 'cb'.join(['1', '2', '3']) + 'c' result = '' for number in ['1', '2', '3']: - result += f"{number}, " # [consider-using-join] + result += f"{number}, " result = result[:-2] diff --git a/tests/functional/c/consider/consider_join_for_non_empty_separator.rc b/tests/functional/c/consider/consider_join_for_non_empty_separator.rc index 3c53da1efe..0d6adeb837 100644 --- a/tests/functional/c/consider/consider_join_for_non_empty_separator.rc +++ b/tests/functional/c/consider/consider_join_for_non_empty_separator.rc @@ -1,2 +1,2 @@ [variables] -suggest-join-with-non-empty-separator=yes +suggest-join-with-non-empty-separator=no diff --git a/tests/functional/c/consider/consider_join_for_non_empty_separator.txt b/tests/functional/c/consider/consider_join_for_non_empty_separator.txt deleted file mode 100644 index 53808b2ac3..0000000000 --- a/tests/functional/c/consider/consider_join_for_non_empty_separator.txt +++ /dev/null @@ -1,4 +0,0 @@ -consider-using-join:5:4:5:26::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED -consider-using-join:11:4:11:26::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED -consider-using-join:17:4:17:27::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED -consider-using-join:23:4:23:27::Consider using str.join(sequence) for concatenating strings from an iterable:UNDEFINED From f9ff7d76cd39462a16c841062b0ac818d3f67b2a Mon Sep 17 00:00:00 2001 From: theirix Date: Mon, 9 Oct 2023 12:50:44 +0300 Subject: [PATCH 10/10] Update documentation for new option --- doc/user_guide/configuration/all-options.rst | 4 ++-- doc/whatsnew/fragments/8701.feature | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/user_guide/configuration/all-options.rst b/doc/user_guide/configuration/all-options.rst index 68b4b7515d..94d2c1775e 100644 --- a/doc/user_guide/configuration/all-options.rst +++ b/doc/user_guide/configuration/all-options.rst @@ -1201,7 +1201,7 @@ Standard Checkers *Let 'consider-using-join' be raised when the separator to join on would be non-empty (resulting in expected fixes of the type: ``"- " + " - ".join(items)``)* -**Default:** ``False`` +**Default:** ``True`` @@ -1219,7 +1219,7 @@ Standard Checkers never-returning-functions = ["sys.exit", "argparse.parse_error"] - suggest-join-with-non-empty-separator = false + suggest-join-with-non-empty-separator = true diff --git a/doc/whatsnew/fragments/8701.feature b/doc/whatsnew/fragments/8701.feature index 4bb7f6406f..4a9781accf 100644 --- a/doc/whatsnew/fragments/8701.feature +++ b/doc/whatsnew/fragments/8701.feature @@ -1,3 +1,3 @@ -Skip ``consider-using-join`` check for non-empty separators unless a ``suggest-join-with-non-empty-separator`` option is set to ``yes``. +Skip ``consider-using-join`` check for non-empty separators if an ``suggest-join-with-non-empty-separator`` option is set to ``no``. Closes #8701