Don't report trailing-whitespaces within strings

[DanielNoord]

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Add a ChangeLog entry describing what your PR does.
• If it's a new feature, or an important bug fix, add a What's New entry in
  doc/whatsnew/<current release.rst>.
• Write a good description on what the PR does.

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes #3822.

[DanielNoord]

Ah too bad. That function is also responsible for other messages. Back to the drawing board.

[coveralls]

Pull Request Test Coverage Report for Build 2910662722

• 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.0003%) to 95.304%

---

Totals
Change from base Build 2907900553:	-0.0003%
Covered Lines:	16925
Relevant Lines:	17759

---

💛 - Coveralls

[Pierre-Sassoulas]

It seems like the check is done on every line, but it could be done only on the last line ? Also what about cross platform line ending (\r\n and \r) for Windows and Mac ?

[DanielNoord]

I'm not sure. I just had to extract this out of the method because there was no simple way to not call the check of trailing-whitespace. I'll try out your suggestion but since we don't have any issues about this I don't think we should also change the line endings that are checked in this PR.

[DanielNoord]

Oh never mind.

I can't extract this. We need to be able to skip the trailing-whitespace check if we added the missing-final-newline. We can do something with a emitted_final_newline variable but I think checking if emitted_final_newline is as quick as if line.endswith("\n") and the current code is a bit clearer.

[Pierre-Sassoulas]

We need to be able to skip the trailing-whitespace check if we added the missing-final-newline.

How about we loop on split_lines[:-1] and handle the last line separately ? I don't like knowingly making a check O(n) when it could be O(1)

[DanielNoord]

I don't think there is a good way without duplicating a big part of code + the comment? There is also the option to do something like offset == len(split_lines) but then again, I'm not sure an endswith is that much slower.

[Pierre-Sassoulas]

What about something like this ?

for offset, line in enumerate(split_lines[-1]):
    self.check_trailing_whitespace_ending(last_line, lineno + offset, line_start):
last_line =  split_lines[-1][1]
offset =  split_lines[-1][0]
if not last_line.endswith("\n"):
    self.add_message("missing-final-newline", line=lineno + offset)
else:
    self.check_trailing_whitespace_ending(last_line, lineno + offset, line_start):

def check_trailing_whitespace_ending(...)
    # We don't test for trailing whitespaces in strings
    # See https://github.com/PyCQA/pylint/issues/6936
    # and https://github.com/PyCQA/pylint/issues/3822
    if tokens.type(line_start) == tokenize.STRING:
        return
    ...

[DanielNoord]

This also doesn't work since split_lines isn't always full, so split_lines[-1] can return an IndexError 😠

I don't think there is anyway to do this without adding additional if statements and assignments which kind of defeat the purpose of calling endswith once.

[Pierre-Sassoulas]

Suggested change

	last_line = split_lines[-1][1]
	offset = split_lines[-1][0]
	if not last_line.endswith("\n"):
	self.add_message("missing-final-newline", line=lineno + offset)

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 87da717

[Pierre-Sassoulas]

I took some time to check the code around this and while I still think something that should be 0(1) is O(n) changing if would require changing the strange regex that I don't understand and seems to match trailing white spaces by also matching \n ... (?!) So it might be possible but it's not going to be easy. Also currently this code is "not broken" so I'm going "not touch it" and trust you on this one 😄