Fix/multiline file boundary issue

[francoisvn]

I discovered a bug where the multiline status of one file can impact a later file. I noticed this with the IndentationCharacterCheck, but I think it would be present in other checks that extend LineCheckBase. No idea if this is the correct way to fix this, but it resolved the issue for me. Unfortunately I can't share the exact code, but I can try create a small test case for reproduction if that would help.

Similar issues, where checks can cross file boundaries, are probably still there with other checks (maybe even still these). It doesn't seem like there is generally anything to prevent against this. A more general solution is to re-create each check with each file (might be slow), or have something like an init() function that gets called before each file, and move most/all of the code in the constructors there.

[codecov-io]

Current coverage is 89.68% (diff: 83.33%)

Merging #277 into dev will increase coverage by 0.05%

@@                dev       #277   diff @@
==========================================
  Files           112        112          
  Lines          7385       7387     +2   
  Methods           0          0          
  Messages          0          0          
  Branches        287        286     -1   
==========================================
+ Hits           6619       6625     +6   
+ Misses          511        508     -3   
+ Partials        255        254     -1   

Powered by Codecov. Last update 9d30630...337ee47

[adireddy]

Thanks @francoisvn. Can you add the tests here pls just to make sure it doesn't break anything.

[francoisvn]

I added 2 tests. The first would have previously failed, but my workaround fixes it (testFileBoundaryIndentation()) and the second still fails (testMultilineQuoteIndentation()).

So there are 2 underlying issues, which I haven't properly addressed in this PR (cause I don't know the codebase well enough):

• Checks can cross file boundaries.
• Multiline detection in LineCheckBase is inherently buggy.

[francoisvn]

Should I maybe remove the test that fails and create an issue for that? It's not a trivial fix, which is why I haven't done it myself.

In addition, it feels like the file boundary issue requires a bit of refactoring to properly fix.

[Gama11]

Yes, can't merge a PR that introduces failing tests.

[francoisvn]

Ok, I've reverted that test and created an issue here: #278

I still think the file boundary issue I've mentioned needs further investigation, but reporting an issue for that doesn't seem sensible, so I don't know how to take it further. I don't know the codebase well enough to know how best to fix it, and it's a non-trivial change, but I think it's important that it gets investigated. Recommendations for what I can do are welcome.

[Gama11]

Thanks!

[francoisvn]

Great. What should I do about the file boundary issue, or is that not important? Should I create an issue? It seems rather important to me, as someone that would like to rely on this tool...

[Gama11]

Creating an issue definitely can't hurt.