STYLE "wrongplaced whitespace" rule doesn't error on its example of a bad string

[MarcoGorelli]

The docstring reads

pandas/scripts/validate_unwanted_patterns.py

Lines 355 to 361 in 9ab55b4

	For example, this is bad:
	>>> rule = (
	... "We want the space at the begging of "
	... "the line if the previous line is ending with a \n "
	... "not at the end, like always"
	... )

yet this file doesn't error if the script is run on it:

$ cat t.py 
rule = (
   "We want the space at the begging of "
   "the line if the previous line is ending with a \n "
   "not at the end, like always"
)
$ python scripts/validate_unwanted_patterns.py -vt 'strings_with_wrong_placed_whitespace' t.py 

Some other files which don't error, though I can't understand why:

$ cat t.py 
rule = (
   "  We want the space at the begging of"
   " the line if the previous line is ending with a \n "
   "not at the end, like always"
)
$ python scripts/validate_unwanted_patterns.py -vt 'strings_with_wrong_placed_whitespace' t.py 

$ cat t.py 
rule = (
   "We want the space at the begging of"
   " the line if the previous line is ending with a  "
   "not at the end, like always"
)
$ python scripts/validate_unwanted_patterns.py -vt 'strings_with_wrong_placed_whitespace' t.py

Hi @ShaharNaveh @datapythonista - do you have any context you could provide on the trailing newline rule? I'm struggling to make sense of it - thanks :)

[MarcoGorelli]

What I don't understand is the logic behind the lines

pandas/scripts/validate_unwanted_patterns.py

Lines 373 to 376 in 9ab55b4

	elif first_line.startswith(" ") or second_line.startswith(" "):
	return False
	elif first_line.endswith(" ") or second_line.endswith(" "):
	return False

Why should they return False? I can't find examples in the codebase of where they would be hit anyway:

$ git diff
diff --git a/scripts/validate_unwanted_patterns.py b/scripts/validate_unwanted_patterns.py
index b6b038ae9d..369039de2c 100755
--- a/scripts/validate_unwanted_patterns.py
+++ b/scripts/validate_unwanted_patterns.py
@@ -370,10 +370,6 @@ def strings_with_wrong_placed_whitespace(
         """
         if first_line.endswith(r"\n"):
             return False
-        elif first_line.startswith("  ") or second_line.startswith("  "):
-            return False
-        elif first_line.endswith("  ") or second_line.endswith("  "):
-            return False
         elif (not first_line.endswith(" ")) and second_line.startswith(" "):
             return True
         return False
$ pre-commit run unwanted-patterns-strings-with-wrong-placed-whitespace --all-files
Check for strings with wrong placed spaces...............................Passed

[datapythonista]

The idea is that on multiline strings we always keep the spaces from a split in the same position (at the end).

foo = ('this is '
       'what we want')
bar = ('and this'
       ' is wrong')

Then, I think there were cases with multiple spaces, like for indenting, inside the string. So we were checking that a single space existed, otherwise we consider the case ok. I don't remember about the line breaks.

I thought these rules were well tested. I'm in my phone now, can't easily check, but aren't there tests that clarify what is doing with examples and make sure this works as expected?

[MarcoGorelli]

Thanks @datapythonista !

The tests miss quite a few lines:

pandas/scripts/validate_unwanted_patterns.py

Lines 87 to 88 in 735f40c

	except ValueError:
	return 0

pandas/scripts/validate_unwanted_patterns.py

Lines 371 to 372 in 735f40c

	if first_line.endswith(r"\n"):
	return False

pandas/scripts/validate_unwanted_patterns.py

Lines 375 to 376 in 735f40c

	elif first_line.endswith(" ") or second_line.endswith(" "):
	return False

pandas/scripts/validate_unwanted_patterns.py

Line 379 in 735f40c

return False

Also, this test:

pandas/scripts/tests/test_validate_unwanted_patterns.py

Lines 212 to 216 in 735f40c

	msg = (
	"foo"
	" bar"
	"baz"
	)

hits the lines

pandas/scripts/validate_unwanted_patterns.py

Lines 373 to 374 in 735f40c

	elif first_line.startswith(" ") or second_line.startswith(" "):
	return False

though none of the pandas codebase does.

I get that if the first string ends with '\n', then we don't want an extra space after it. What's not clear to me is these lines:

pandas/scripts/validate_unwanted_patterns.py

Lines 373 to 376 in 9ab55b4

	elif first_line.startswith(" ") or second_line.startswith(" "):
	return False
	elif first_line.endswith(" ") or second_line.endswith(" "):
	return False

Seeing as nothing in the pandas codebase hits them, I'd be inclined to remove them

[datapythonista]

Seems to me that these two last elif should be just one checking the end of the first line, and the start of the second. Doesn't make much sense this way.

But in any case, that was added for some case that wasn't working with the simple validation of if the space is not at the end of the first line instead of at the beginning of the second, then fail. So, if there is nothing in our code now that fails with that simple condition, agree that better to remove.

[ShaharNaveh]

Seems to me that these two last elif should be just one checking the end of the first line, and the start of the second. Doesn't make much sense this way.

IIRC the reason for those two elif was to be as explicit as possible, in order to cover as much cases as possible I guess it slipped from me that they are not really useful 😅

But in any case, that was added for some case that wasn't working with the simple validation of if the space is not at the end of the first line instead of at the beginning of the second, then fail. So, if there is nothing in our code now that fails with that simple condition, agree that better to remove.

IIRC they did catch something in the codebase back then, but that was a year ago, I'm not really sure.
Any way, I think it would be better to remove them, as long as they don't seems to be needed. You can always add them again whenever you want.