Make sure to split non-separated csv OuputLine's

[DanielNoord]

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Write a good description on what the PR does.

Type of Changes

	Type
✓	🔨 Refactoring

Description

https://github.com/PyCQA/pylint/blob/2631583577a3b2228af66214a364e4e284ba55dd/tests/testutils/test_output_line.py#L126-L130

This example was not actually handled correctly. For some reason issubclass(str, Sequence) equals True, so the isinstance check didn't make any sense. This meant that the above line had a len of 51. This changes it so that a string will first be separated on the comma's and then parsed.
The diff is quite large as the indentation could then be removed.

I also hope this removes some DeprecationWarnings that were showing up in the test suite on 3.7 and 3.8.

[coveralls]

Pull Request Test Coverage Report for Build 1683437463

• 13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
• 29 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.02%) to 93.765%

---

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/classes/class_checker.py	29	94.5%

Totals
Change from base Build 1679862235:	0.02%
Covered Lines:	14482
Relevant Lines:	15445

---

💛 - Coveralls

[Pierre-Sassoulas]

👍

[Pierre-Sassoulas]

Maybe we could remove the error handling ? There's no index error anymore. It could still happen with malformed output file.

[DanielNoord]

I think the IndexError mimics refers to the fact that the Sequence or list does not have the correct length. Do you want me to remove it?

[Pierre-Sassoulas]

Let's keep that un mind for 3.0, it's a breaking change for plugin development after all. Would you mind adding a todo comment close to the deprecation warnings ?

[DanielNoord]

Let's keep that un mind for 3.0, it's a breaking change for plugin development after all.

Really? The IndexError will never be raised as it caught by the except, right?

Would you mind adding a todo comment close to the deprecation warnings ?

I did a quick search for DeprecationWarning and there are a lot of them without TODO's. I'm fine with adding them, but perhaps we should just remind ourselves to look at all DeprecationWarning's before releasing 3.0 and not put TODO's everywhere?

[Pierre-Sassoulas]

No I meant a todo to remove this particular index error handling. Or do you think it's obvious ? The coverage decreased right now si it's easy to see the code became unused but we might not realize it later. I don't think we should add Todos for all deprecation warning.

Sorry I'm on mobile so I can't be very detailled. I think the index error can happen on plugin code using our functional tests without enough elements in the output.

[DanielNoord]

Oh wait, there is another IndexError that has lost coverage. I didn't catch that. I thought you were referring to the IndexError in from_csv.

Looking into it I don't think the test is testing what it used to test. We raising a ValueError on the first two cases because it tries to turn a "'1'" into an integer, which is impossible. This creates the MalformedOutputLineException. I feel like previously the tests were used to test different lengths, but oh well. They still pass...
I have added a test to make sure the IndexError is covered again. Basically we allow any OutputLine that is too long there so that we can still generate the nice error message even if it is too long. I don't think we should disallow that. If you misplace a : or , when hand-writing the OutputLine it would be nice to still get a formatted error message instead of a new IndexError. This test tests that behaviour.

[Pierre-Sassoulas]

I have added a test to make sure the IndexError is covered again. Basically we allow any OutputLine that is too long there so that we can still generate the nice error message even if it is too long.

I remember know. I wrote that code, I'm the one responsible for the lack of tests.. 😓

Thanks for covering that !