Clean-up blank lines left behind by EncapsulateFieldRefactoring and MoveCloserToUsage #5555
Conversation
Extends the IModuleRewriter.Remove() method to clear residual blank lines between the removed declaration and the next declaration. Supports VariableDeclarations and ModuleBodyElementDeclarations removals. Also improves Variable declaration list handling.
Incorporates IModuleRewriter extension method to clean up newlines left by declaration removals. Removes EncapsulateFieldRewriteSession wrapper class that previously handled declaration removals.
Incorporates IModuleRewriter extension method to clean up newlines left by declaration removals.
| /// </remarks> | ||
| public static void RemoveVariables(this IModuleRewriter rewriter, IEnumerable<VariableDeclaration> toRemove, bool removeEndOfStmtContext = true) | ||
| { | ||
| if (!toRemove.Any()) { return; } |
There was a problem hiding this comment.
To follow the style conventions in this project, please put the return; on its own line.
| if (!toRemove.Any()) { return; } | ||
|
|
||
| var fieldsByListContext = toRemove.Distinct() | ||
| .GroupBy(f => f.Context.GetAncestor<VBAParser.VariableListStmtContext>()); |
There was a problem hiding this comment.
The general style used in the project is to indent method calls on new lines one further than the previous line.
|
|
||
| namespace Rubberduck.Refactorings.Common | ||
| { | ||
| public static class IModuleRewriterExtensions |
There was a problem hiding this comment.
I am not a fan of extension methods since they come with a few drawbacks.
- You cannot swap them out via a different implementation.
- You cannot mock them in tests.
However, using them here might be OK. There really is no sense in providing an alternate implementation here and we never assert on the calls to the rewriters in tests; we assert on the result, instead. To me the question remains whether the new methods should actually be part of the IModuleRewriter interface in the first place. Opinions?
There was a problem hiding this comment.
Since RD 'owns' ModuleRewriter, adding the methods to IModuleRewriter is an available choice and certainly avoids the drawbacks you mention. Even though I used an extension class, I'm not advocating that extension methods are the best choice here. For a PR submission, I am very reluctant to modify such a ubiquitous interface as IModuleRewriter without specific direction...So, extension methods provide a less impactful way to initially add/propose new behavior. I'm good with making the extension methods part of the IModuleRewriter if that is preferred.
| /// </remarks> | ||
| public static void RemoveMembers(this IModuleRewriter rewriter, IEnumerable<ModuleBodyElementDeclaration> toRemove, bool removeEndOfStmtContext = true) | ||
| { | ||
| if (!toRemove.Any()) { return; } |
|
|
||
| foreach (var fieldsGroup in fieldsByListContext) | ||
| { | ||
| var variables = fieldsGroup.Key.children.Where(ch => ch is VBAParser.VariableSubStmtContext); |
There was a problem hiding this comment.
This would probably look cleaner as OfType<VBAParser.VariableSubStmtContext>().
| var variables = fieldsGroup.Key.children.Where(ch => ch is VBAParser.VariableSubStmtContext); | ||
| if (variables.Count() == fieldsGroup.Count()) | ||
| { | ||
| if (fieldsGroup.First().ParentDeclaration.DeclarationType.HasFlag(DeclarationType.Module)) |
There was a problem hiding this comment.
This is a second iteration of fieldsGroup. Accordingly, it should be materialized via ToList where it is defined.
There was a problem hiding this comment.
I understand the comment..though I am curious as how to best materialize the List since fieldsGroup is defined within a foreach (var fieldsGroup in fieldsByListContext) statement. Currently, I've added var fieldsToDelete = fieldsGroup.ToList(); and use fieldsToDelete for the Count() and First() calls which (I believe) resolves the comment.
Is there another more preferred way in this scenario?...perhaps using ToLookup to instantiate fieldsByListContext instead of GroupBy resolves this as well without calling ToList on fieldsGroup?
There was a problem hiding this comment.
That resolves the issue. You could alternatively use ToDictionary on fieldsByListContext. There should be an extension method turning it into a Dictionary<TKey, List<TValue>>.
|
|
||
| refactorRewriteSession = RefactorRewrite(model, refactorRewriteSession); | ||
| RefactorRewrite(model, refactorRewriteSession as IRewriteSession); |
There was a problem hiding this comment.
The cast seems to be redundant; the return type of CheckOutCodePaneSession extends IRewrtiteSession.
Or is there another overload taking an IExecutableRewriteSession?
| { | ||
| refactorRewriteSession.Remove(field.Declaration, rewriter); | ||
| } | ||
| rewriter.RemoveVariables(SelectedFields.Select(f => f.Declaration).Cast<VariableDeclaration>()); |
There was a problem hiding this comment.
I would add a line break for each LINQ call. However, that is a question of style.
| InsertNewDeclaration(target, rewriteSession); | ||
| RemoveOldDeclaration(target, rewriteSession); | ||
| UpdateQualifiedCalls(target, rewriteSession); | ||
| if (target is VariableDeclaration variable) |
There was a problem hiding this comment.
That should probably be inverted. The result is a guard clause and no need to use return.
Retains use of IModuleRewriterExtensions class.
|
Code is updated per the comments. Retained the |
There was a problem hiding this comment.
Not a huge fan of extension methods against our own types, but as Max explained this particular case does not interfere with testing, ...and I'm thinking this kind of methods could ultimately live in a set of extension namespaces or something, kind of like LINQ or Dapper, but to provide such specialized methods to RD's IModuleRewriter; we would have to think of a way to regroup them all so that they're easily reusable and we don't end up reinventing that wheel - I love all the extensive xmldoc going on here, great work as always!
The
EncapsulateFieldRefactoringpotentially leaves large blocks of blank lines in the scenarios when the user elects to encapsulate numerous fields by wrapping them in a UDT.Residual newlines also occur in the
MoveCloserToUsagerefactoring and quickfix. It is less noticeable since these actions delete declarations one at a time. But, running the refactoring/quickfix against many declarations in the same module can potentially result in a large block of residual blank lines.To accomplish the newline removals, this PR introduces an
IModuleRewriterExtensionsclass in Refactorings.Common. The purpose of the extension methods are to provide a convenient mechanism to clear residual newlines left behind by the removal of variable and member declarations.IModuleRewrite.Removedoes not remove theEndOfStmtContextwhich can result in orphaned newlines. The extension method related to variable declarations also correctly handles removal of a variable declaration list in the scenario where are all variables in the list are identified for removal. (callingIModuleRewrite.Removeon each variable in a list leaves theVisibilityContextbehind)