Move closer refactoring indent fix #3973
Conversation
Codecov Report
@@ Coverage Diff @@
## next #3973 +/- ##
==========================================
+ Coverage 57.81% 57.81% +0.01%
==========================================
Files 843 843
Lines 30063 30069 +6
==========================================
+ Hits 17378 17384 +6
Misses 12685 12685
|
|
For the Record: When fixing the original bug I tried to implement this on the parse tree, but gave it up. |
There was a problem hiding this comment.
Thanks for your first PR! We love having new contributors. My concerns are mainly on 2 areas:
- avoiding memory leaks that can result from direct COM access
- having unit tests to prove the assumptions where there are line numbers or any other in front that might not be appropriate for indentation.
The memory leak is pretty important, which is why we need to make changes before it can be merged.
| @@ -168,9 +169,12 @@ private void InsertNewDeclaration() | |||
| } | |||
|
|
|||
| var insertionIndex = (expression as ParserRuleContext).Start.TokenIndex; | |||
| var firstReferenceLine = _vbe.ActiveCodePane.CodeModule.GetLines((expression as ParserRuleContext).Start.Line, 1); | |||
There was a problem hiding this comment.
_vbe., ActiveCodePane, and CodeModule each are a SafeComWrapper. Those must be wrapped in a using block for each reference to ensure proper release of the wrapper.
There was a problem hiding this comment.
PS I spoke a bit too hastily - _vbe doesn't need to be in the using; the other two do, though.
There was a problem hiding this comment.
Thanks, will update with a pair of using blocks
| @@ -168,9 +169,12 @@ private void InsertNewDeclaration() | |||
| } | |||
|
|
|||
| var insertionIndex = (expression as ParserRuleContext).Start.TokenIndex; | |||
| var firstReferenceLine = _vbe.ActiveCodePane.CodeModule.GetLines((expression as ParserRuleContext).Start.Line, 1); | |||
| var indentLength = firstReferenceLine.Length - firstReferenceLine.TrimStart().Length; | |||
| var padding = new string(' ', indentLength); | |||
There was a problem hiding this comment.
I observe that none of unit tests account for the case where there is something that isn't "indentable" -- such as line numbers for example. Thus, what happens when we have code like this?
100 Foo = bar
The assumption here is that we only indent if there is only whitespace on the line and that might be actually fine but the unit tests should prove that assumption.
There was a problem hiding this comment.
Very good point, but IMO not a showstopper - let's get the wrappers into using blocks, and if the line number stuff can't be fixed right away I'd still merge it.
There was a problem hiding this comment.
I've added a new test and it moves the line with the line number down without introducing any indent before the number so I think that's okay.
However, I tried a different test where the declaration has a line number. The declaration gets moved but leaves the line number behind. This is a problem when the line after the declaration has a line number too and suddenly we have a line with two line numbers! This is unrelated to the changes I made, so I will open a new issue.
|
|
||
| var rewriter = _state.GetRewriter(firstReference.QualifiedModuleName); | ||
| rewriter.InsertBefore(insertionIndex, newVariable); | ||
| rewriter.InsertBefore(insertionIndex, newVariable + padding); |
There was a problem hiding this comment.
I wonder if given the fact that we create a newVariable which ends with a newline, it means that we can just look at the expression's Column property and pad that many? That would avoid the COM access altogether.
There was a problem hiding this comment.
This was how I tried it first but it gave large indents in the case where the expression it is inserted before follows another and separated by a colon. E.g. in the test case
Private foo As Long
Public Sub Test(): SomeSub someParam:=foo: End SubBecomes
Public Sub Test(): Dim foo As Long
SomeSub someParam:=foo: End SubI think it would cause issues with the line number example as well for similar reasons.
|
Good job. Feel free to join us in our war room: https://chat.stackexchange.com/rooms/14929/vba-rubberducking |
Fixes #3964
First line after insertion was given no indentation. This PR checks the indentation of the line prior to the insertion and adds padding so that the same indentation is preserved.
A reparse is also requested after the refactoring has completed.