Remove Regex-Replaces/Raw String ops from #2660 part 2 #2733

BZngr · 2017-02-23T20:56:51Z

Follow up to rejected PR to #2714. Updates AssignedByValParameterQuickFixes to use ParseTree results. Incorporates comments from PR #2174. Adds an ICodeModuleExtensions class per PR #2714 comments.

… QuickFixHelper

…e it a static class

…class file

retailcoder · 2017-02-23T22:36:41Z

RetailCoder.VBE/Common/ICodeModuleExtensions.cs

+
+namespace Rubberduck.Common
+{
+    public static class ICodeModuleExtensions


The I prefix indicates an interface; I'd rename this one to CodeModuleExtensions; this is similar to LINQ's Enumerable static class, which extends the IEnumerable interface.

Will make the change.

retailcoder · 2017-02-23T22:38:08Z

RetailCoder.VBE/Common/ICodeModuleExtensions.cs

+            var result = ReplaceStringAtIndex(original, token.Text, replacement, token.Column);
+            module.ReplaceLine(token.Line, result);
+        }
+        public static void ReplaceIdentifierReferenceName(this ICodeModule module, IdentifierReference identifierReference, string replacement)


Nitpick: I like to have an empty line between members of a class - that would be consistent with the rest of the code base.

That said the class looks pretty neat otherwise 👍

Will make the change.

retailcoder · 2017-02-23T22:40:24Z

RetailCoder.VBE/Inspections/QuickFixes/AssignedByValParameterMakeLocalCopyQuickFix.cs

+        public override bool CanFixInProject { get { return false; } }
+
+        //This function exists solely to support unit testing 
+        public void TESTONLY_FixUsingAutoGeneratedName(string suggestedName = "")


I don't like this... a class shouldn't have the knowledge of whether or not it's being executed in a test context. There has to be a better way.

Take a look at the RenameDeclarationQuickFix and associated tests for an example of how to approach this. The "testing only" part is basically just to suppress the dialog - if you make the dialog a dependency, you can mock it in the tests.

retailcoder · 2017-02-23T22:57:30Z

RetailCoder.VBE/Inspections/QuickFixes/AssignedByValParameterMakeLocalCopyQuickFix.cs

+            {
+                view.NewName = _localCopyVariableName;
+                view.IdentifierNamesAlreadyDeclared = _variableNamesAccessibleToProcedureContext;
+                if (!_isQuickFixUnitTest)


The problem is that there's this AssignedByValParameterQuickFixDialog dependency that needs to be tamed.

You could constructor-inject an abstract factory here - and Ninject will do all the hard work!

public interface IAssignedByValParameterQuickFixDialogFactory { IAssignedByValParameterQuickFixDialog Create(Declaration target, Selection selection, string[] accessibleNames); }

Just make the dialog implement an interface that's named after it (with the I prefix) - that interface could look like this:

public interface IAssignedByValParameterQuickFixDialog : IDisposable { void ShowDialog(); DialogResult DialogResult { get; } string NewName { get; } }

Make the dialog implement that interface (just tacking : IAssignedByValParameterQuickFixDialog to its definition should be enough, from what I'm seeing)

And then the AssignedByValParameterMakeLocalCopyQuickFix can take an IAssignedByValParameterQuickFixDialogFactory constructor parameter and store that abstract factory in a private readonly field:

private readonly IAssignedByValParameterQuickFixDialogFactory _dialogFactory;

Now, instead of doing using (var view = new AssignedByValParameterQuickFixDialogFactory(_target, Selection)), you can do this:

using (var view = _dialogFactory.Create(_target, Selection, _variableNamesAccessibleToProcedureContext))

And now whoever creates a AssignedByValParameterMakeLocalCopyQuickFix is responsible for passing the dependencies to the constructor - I think that would be the inspection result class... so for now let's make that inspection result's constructor intake the IAssignedByValParameterQuickFixDialogFactory dependency through its constructor, and then since the inspection result is new'd up by the inspection itself, the inspection's constructor for now will have to intake the IAssignedByValParameterQuickFixDialogFactory dependency through its constructor as well.

That's dirty dependency injection, but it should "just work". We'll revisit the injecting depedencies of dependencies of dependencies issue when the inspections feature is re-architected.

Forgot to mention: the abstract factory doesn't need a concrete implementation - Ninject will deal with that automatically.

Thanks for providing all this detail. I'll look at the examples and get rid of the dependency (I was a little surprised this topic didn't come up in the very first PR!).

Vogel612 · 2017-02-23T22:50:10Z

RetailCoder.VBE/Inspections/QuickFixes/AssignedByValParameterMakeLocalCopyQuickFix.cs

+            var firstBlockLineNumber =  blocks.FirstOrDefault().Start.Line;
+
+            var module = Selection.QualifiedName.Component.CodeModule;
+            module.InsertLines(firstBlockLineNumber++, BuildLocalCopyDeclaration());


why the ++ here? it might be clearer to extract that into a separate statement.

Alternatively you could join the two declarations and insert them together

I can extract it. Originally, I had a CodeModuleExtension function InsertLinesAfter( int lineNumber, string[] lines) but thought it might be overkill for inserting just two lines. Would putting a function like this in the CodeModule extension class make sense in general?

Vogel612 · 2017-02-23T22:54:31Z

RetailCoder.VBE/Inspections/Resources/InspectionsUI.resx

@@ -647,7 +647,7 @@ If the parameter can be null, ignore this inspection result; passing a null valu
  <data name="ApplicationWorksheetFunctionQuickFix" xml:space="preserve">
    <value>Use Application.WorksheetFunction explicitly.</value>
  </data>
-  <data name="AssignedByValParameterQuickFix" xml:space="preserve">
+  <data name="AssignedByValParameterMakeLocalCopyQuickFix" xml:space="preserve">


Note: needs to also be adjusted in the other languages

There were not pre-existing entries in fr.resx and de.resx to replace for 'AssignedByValParameterQuickFix'. So, I'm planning to add the entry with the english description unless someone can provide translations...Or, if it is better to leave them missing than entered in english. Thanks.

If they don't exist yet, you may as well leave them missing ... Because then the translators for the respective languages (me and retailcoder) need to get to those either way. On that note: @retailcoder we should probably do a i18n check for the next release either way ...

retailcoder

👍

…duck-vba/next' into next

…rMakeLocalCopyQuickFix

BZngr · 2017-02-24T18:22:28Z

This version removes the QuickFix's dependency on the dialog class. Used two implementations of a factory interface to accomplish the result. Could not get figure out how to get there with Mock so created a simple Mock class of my own. At the bottom of AssignedByValParameterMakeLocalCopyQuickFixTests.cs you'll find the test-related factory implementation and Mocked class.

retailcoder · 2017-02-25T04:57:54Z

RubberduckTests/Inspections/AssignedByValParameterMakeLocalCopyQuickFixTests.cs

    [TestClass]
-    public class AssignedByValParameterMakeLocalCopyQuickFixTests
+    public class AssignedByValParameterMakeLocalCopyQuickFixTests : IAssignedByValParameterQuickFixDialogFactory


The test class implements the factory interface... why?

The QuickFix calls "Create(...)" on the factory interface passed into the constructor. During unit testing, the Testing class passes in a factory interface that points to itself so that it can construct a and return an interface to the MockDialog in order to support the tests.

retailcoder · 2017-02-25T05:04:03Z

RetailCoder.VBE/UI/Refactorings/IAssignedByValParameterQuickFixDialog.cs

+
+namespace Rubberduck.UI.Refactorings
+{
+    public interface IAssignedByValParameterQuickFixDialog : IDialogView


👍 extends IDialogView

retailcoder · 2017-02-25T05:05:56Z

RetailCoder.VBE/Inspections/Results/AssignedByValParameterInspectionResult.cs

@@ -25,9 +26,10 @@ public override IEnumerable<QuickFixBase> QuickFixes
        {
            get
            {
+                IAssignedByValParameterQuickFixDialogFactory factory = new AssignedByValParameterQuickFixDialogFactory();


The factory is a dependency, it should be constructor-injected.

So, do you mean moving this (or a similar) line of code to the constructor of the Inspection class? I'm OK with moving it....I'm wondering if the constructor of the inspection should carry the factory interface dependency for a dialog component of one of the three quickfixes - it seems a little 'high up' - but it's likely I'm not understanding what you are advocating. Could you comment a bit more? Thanks.

I went back and reviewed your earlier comments on this topic...I can see that they are consistent with the above comment - I think I just lost track of where to bring in the dependency once I started coding. Soooo...I'll have another update coming soon.

This latest update to the PR (2/25) is much closer (I'm hoping ) to what you expected to see. The couple lines of code I added to RubberDuckModule.cs look out of place to me, so I suspect there is a better way. As I'm sure you've already figured out, using Ninject is new to me...just another one of many things here at RD!.

…vba/next' into next

retailcoder · 2017-02-27T14:44:12Z

RetailCoder.VBE/Root/RubberduckModule.cs

@@ -425,6 +429,10 @@ private IEnumerable<ICommandMenuItem> GetRubberduckCommandBarItems()

        private IEnumerable<IMenuItem> GetRubberduckMenuItems()
        {
+            //This bind needs to occur before the the new array is built by the function calls


That's because the factory interface naming convention is kicking in and binding to an automatic factory implementation; if you want to override that binding you can use Rebind - please keep GetRubberduckMenuItems dedicated to returning the RD menu items.

Thanks for the feedback on this particular change. Using Rebind did not succeed...I'm thinking it is because the Inspection objects are constructed just once (when the menu items are created) and there is never another opportunity to inject the redefined dependency with Rebind(). (True?) I've moved the Bind definition to a sequentially (and contextually) correct place in the body of the "Load()' function.

retailcoder · 2017-02-27T14:45:39Z

RetailCoder.VBE/UI/Refactorings/AssignedByValParameterQuickFixMockDialogFactory.cs

+
+namespace Rubberduck.UI.Refactorings
+{
+    public class AssignedByValParameterQuickFixMockDialogFactory : IAssignedByValParameterQuickFixDialogFactory


We don't need to make actual mock implementations to inject in the tests - we use a mocking framework for that (Moq).

I'll figure out how to use the Moq framework.

retailcoder · 2017-02-27T14:53:42Z

RetailCoder.VBE/Inspections/QuickFixes/AssignedByValParameterMakeLocalCopyQuickFix.cs

@@ -49,7 +49,7 @@ public override void Fix()

        private void RequestLocalCopyVariableName()
        {
-            using( var view = _factory.Create(_target.IdentifierName, _target.DeclarationType.ToString()))
+            using( var view = _dialogFactory.Create(_target.IdentifierName, _target.DeclarationType.ToString()))


isn't this blowing up with a null reference exception in the tests when you pass a null factory to the inspection? You'll want to give it a Mock<IAssignedByValParameterQuickFixDialogFactory> object, something like this:

var mock = new Mock<IAssignedByValParameterQuickFixDialogFactory>(); mock.SetupGet(m => m.NewName).Returns(() => WhateverTheTestExpectsTheNewNameToBe); mock.SetupGet(m => m.IdentifierNamesAlreadyDeclared).Returns(() => WhateverTheTestNeedsToWorkWith); mock.SetupGet(m => m.IsCancelled).Returns(() => false); // or true, to test cancellation behavior var inspection = new AssignedByValParameterInspection(mockParser.State, mock.Object);

All the tests pass free of any exceptions - The factory reference is not de-referenced unless the test invokes "Fix()" on the MakeLocalCopyQuickFix. Once I sort out to use the Moq framework for the factory - I'll pass it to the constructor.

@BZngr ah, sorry I missed that! don't bother setting up a mock for a test that doesn't need it ;-)

I guess a mock will be useful for a test that tests the quickfix itself.

…e Load() function

retailcoder · 2017-02-27T22:14:16Z

I don't have push access to your fork so I can't fix the merge conflict from here, but it should be a pretty simple one to fix 😃

Let me know when you're ready to merge the PR!

BZngr · 2017-02-27T22:27:31Z

Yes - the conflicts are straight forward - locally I've already resolved them. Yet to add the code to use the Moq framework. Hoping to have time to do it later today. Thanks.

…bberduck-vba/next' into next

BZngr · 2017-02-28T02:10:32Z

All comments addressed - I'm thinking it's finally ready to go.

Vogel612 · 2017-02-28T10:00:20Z

Currently failing tests on AppVeyor:

EnclosingProjectComesBeforeOtherProceduralModule
EnclosingProjectComesBeforeOtherModuleInEnclosingProject
GivenLocalDeclarationAsQualifiedClassName_ResolvesFirstPartToProject

I don't know in which way that was caused by this PR ...

BZngr · 2017-02-28T16:03:14Z

When I look at the content of these tests, any relationship to this PR is not obvious to me. The tests appear to be independent of code that was modified. I also ran these tests in loops. The results vary slightly with each run of the loops. However, the results below are typical:

EnclosingProjectComesBeforeOtherProceduralModule

Run Mode: succeeded 60 out of 100 tests
Debug Mode: succeeded 92 out of 100 tests

EnclosingProjectComesBeforeOtherModuleInEnclosingProject

Run Mode: succeeded 63 out of 100 tests
Debug Mode: succeeded 91 out of 100 tests

GivenLocalDeclarationAsQualifiedClassName_ResolvesFIrstPartToProject

Run Mode: succeeded 55 out of 100 tests
Debug Mode: succeeded 90 out of 100 tests

MDoerner · 2017-02-28T16:09:25Z

@BZngr Do not worry about the failed tests. These are three of our beloved six tests that fail an pass randomly for about two months now. We still have not figured out what is causing it but it must be some concurrency issue in the tests.

BZngr · 2017-02-28T17:30:20Z

FWIW - There is a simple way to mask the concurrency issue yet allow the tests to run and detect 'hard' failures. Just give the Parser several tries to get the right answer prior to evaluating the results within the Assert. The example below is for 'EnclosingProjectComesBeforeOtherProceduralModule':

for (int idx = 1; idx < 25 && declaration.References.Count() != 1; idx++)
{
     state = Parse(vbe);
     declaration = state.AllUserDeclarations
          .Single(d => d.DeclarationType == DeclarationType.Project 
              && d.IdentifierName == BINDING_TARGET_NAME);
}

This approach works for all three tests. The loop is different for GivenLocalDeclarationAsQualifiedClassName_ResolvesFIrstPartToProject, but the principle is the same.

BZngr · 2017-03-01T02:06:04Z

Looking for some input. The PR as it stands is an improvement over the version of the QuickFix currently in 'next'. That said, there is still some work to do to expand the assistance to the user in choosing a new name. That is, to include module scope and global scope variable name checks. I am currently working on this. So...my question is regarding the preference at this point - and I am good with either. Do you want to hold off on this PR - and I'll continue to push improvements as I have them ready? Or, proceed as it is to get the current improvements and identify the additional checks as a bug. Just wanted to make sure it was clear what is missing (as far as I know) from the PR as it currently exits. Thanks.

retailcoder · 2017-03-01T02:52:37Z

@BZngr sorry for the delay! no worries, I just hadn't had a chance to get to it yet. On it!

#2733 post-merge tweaks

BZngr added 5 commits February 23, 2017 08:24

Forced repo to match RD\next. Reintroduced LocalVariable QuickFix and…

d687a03

… QuickFixHelper

new QuickFix files reintroduced

6136377

Created ICodeModuleExtensions class. Minimized QuickFixHelper and mad…

c857f06

…e it a static class

Removed references to AssignedByValParameterQuickFix and deleted the …

43f9c67

…class file

Add dedicated test class for PassParameterByReferenceQuickFix

5985c6a

BZngr mentioned this pull request Feb 23, 2017

Remove Regex-Replaces/Raw String ops from #2660 (AssignedByValParameterQuickFix) #2714

Closed

retailcoder reviewed Feb 23, 2017

View reviewed changes

Vogel612 reviewed Feb 23, 2017

View reviewed changes

Addressed PR initial comments except Dialog dependency removal

b151934

retailcoder reviewed Feb 24, 2017

View reviewed changes

BZngr added 2 commits February 23, 2017 21:44

Synced with rubberduck-vba/next. Merge remote-tracking branch 'rubber…

8cd219b

…duck-vba/next' into next

Removed concrete Dialog class dependency within AssignedByValParamete…

6d3c228

…rMakeLocalCopyQuickFix

MDoerner mentioned this pull request Feb 24, 2017

Remove references of not reparsed components. #2738

Merged

retailcoder reviewed Feb 25, 2017

View reviewed changes

BZngr added 4 commits February 24, 2017 21:45

Sync with latest merged PR. Merge remote-tracking branch 'rubberduck-…

69ddd7f

…vba/next' into next

Implemented as abstract factory pattern per comments

e21a5ad

Update from laest PR merge. Merge remote-tracking branch 'rubberduck-…

439aaac

…vba/next' into next

shortened ReplaceStringAtIndex for refactorRename example

b0d3911

retailcoder reviewed Feb 27, 2017

View reviewed changes

BZngr added 2 commits February 27, 2017 09:49

Moved AssignedByValParameterQuickFixDialogFactory Bind() call into th…

d9af7aa

…e Load() function

resolved merge conflicts

afdc09c

BZngr added 3 commits February 27, 2017 14:27

Synced with latest PRs merged to RD. Merge remote-tracking branch 'ru…

6871731

…bberduck-vba/next' into next

Final updates per PR comments

8e4a56b

Minor fix

a6fb323

retailcoder merged commit 359cbcf into rubberduck-vba:next Mar 1, 2017

retailcoder added a commit that referenced this pull request Mar 1, 2017

Merge pull request #2761 from retailcoder/rd-next

427ef60

#2733 post-merge tweaks

Remove Regex-Replaces/Raw String ops from #2660 part 2 #2733

Remove Regex-Replaces/Raw String ops from #2660 part 2 #2733

Conversation

BZngr commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retailcoder left a comment

Choose a reason for hiding this comment

BZngr commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retailcoder commented Feb 27, 2017

BZngr commented Feb 27, 2017

BZngr commented Feb 28, 2017

Vogel612 commented Feb 28, 2017

BZngr commented Feb 28, 2017

MDoerner commented Feb 28, 2017

BZngr commented Feb 28, 2017 • edited by Vogel612

BZngr commented Mar 1, 2017

retailcoder commented Mar 1, 2017

BZngr commented Feb 28, 2017 •

edited by Vogel612