Disable consider-using-join for non-empty separator

[vmeurisse]

Current problem

In 2.16.0, consider-using-join was extended to include non-empty separators. This makes pylint complains about the following code:

def test(values: List[str]):
    data = ""
    for value in values:
        data += f"prefix{value}suffix"
    return data

However I don't think the suggested refactor is an improvement: prefix and suffix are duplicated and the empty list case need to be handled specifically

def test(values: List[str]):
    if not values:
        return ""
    return "prefix" + "suffixprefix".join(values) + "suffix"

The current options are:

• disable the rule completely. This has the drawback of loosing the previous rule for empty separators.
• use a "pylint: disable" comment. However having to use this on regular basis creates bad habits and reduce the value of linting

Desired solution

Would it be possible to have an option to disable the rule only for the non-empty separator case?

Additional context

No response

[Pierre-Sassoulas]

I remember having the same sentiment during initial implementation too. I think adding an option make sense.

[vivekthedev]

I am interested in solving this one can you guide me through it?

[Pierre-Sassoulas]

The checking code is here:

pylint/pylint/checkers/refactoring/refactoring_checker.py

Lines 1696 to 1731 in 2c10a85

	def _check_consider_using_join(self, aug_assign: nodes.AugAssign) -> None:
	"""We start with the augmented assignment and work our way upwards.
	Names of variables for nodes if match successful:
	result = '' # assign
	for number in ['1', '2', '3'] # for_loop
	result += number # aug_assign
	"""
	for_loop = aug_assign.parent
	if not isinstance(for_loop, nodes.For) or len(for_loop.body) > 1:
	return
	assign = for_loop.previous_sibling()
	if not isinstance(assign, nodes.Assign):
	return
	result_assign_names = {
	target.name
	for target in assign.targets
	if isinstance(target, nodes.AssignName)
	}
	is_concat_loop = (
	aug_assign.op == "+="
	and isinstance(aug_assign.target, nodes.AssignName)
	and len(for_loop.body) == 1
	and aug_assign.target.name in result_assign_names
	and isinstance(assign.value, nodes.Const)
	and isinstance(assign.value.value, str)
	and self._name_to_concatenate(aug_assign.value) == for_loop.target.name
	)
	if is_concat_loop:
	self.add_message("consider-using-join", node=aug_assign)
	@utils.only_required_for_messages("consider-using-join")
	def visit_augassign(self, node: nodes.AugAssign) -> None:
	self._check_consider_using_join(node)

You can add option here:

pylint/pylint/checkers/refactoring/refactoring_checker.py

Lines 489 to 511 in 2c10a85

	options = (
	(
	"max-nested-blocks",
	{
	"default": 5,
	"type": "int",
	"metavar": "<int>",
	"help": "Maximum number of nested blocks for function / method body",
	},
	),
	(
	"never-returning-functions",
	{
	"default": ("sys.exit", "argparse.parse_error"),
	"type": "csv",
	"metavar": "<members names>",
	"help": "Complete name of functions that never returns. When checking "
	"for inconsistent-return-statements if a never returning function is "
	"called then it will be considered as an explicit return statement "
	"and no message will be printed.",
	},
	),
	)

And the functional tests are in this file. (To launch only the functional tests you can do pytest -k consider_join).

Let me know if there's anything else I can do to help :)

[vivekthedev]

Hey @Pierre-Sassoulas Thanks for providing the code sections. I have created the option but how should I go about modifying the checking. Should I write a new function or detect non-empty separator in the same function and return the function if found. I am new to contributing to large codebases.

[Pierre-Sassoulas]

I think the _check_consider_using_join function can be tweaked a little to detect the non empty separator we want to avoid raising for if the suggest-join-with-non-empty-separator is true.

[vivekthedev]

Hey @Pierre-Sassoulas Apologies for late response. In this problem do we have to check the code for non-empty separators with astroid or just adding the the if statement like this is enough:

 if self.linter.config.suggest_join_with_non_empty_separator:
            return

[theirix]

Hello. I have tried to implement this feature, please take a look.