Issue4282 import module folder annotation #4444

IvenBach · 2018-10-16T23:15:02Z

Close #4282. I've tested with .bas and .cls modules and had them import successfully, forms I've not dealt with.

I plan to encapsulate whatever crossover logic there is for #3332. Are there any glaring issues with what I have so far?

Rubberduck.Core/UI/CodeExplorer/CodeExplorerControl.xaml

retailcoder · 2018-10-16T23:55:44Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

+                var tempHelper = (CodeExplorerItemViewModel)parameter;
+                var updatedFolder = (parameter is CodeExplorerCustomFolderViewModel) ? tempHelper.Name : tempHelper.GetSelectedDeclaration().CustomFolder;
+
+                const string folderAnnotation = "'@Folder";


per VBAParserAnnotationFactory, this should probably be AnnotationType.Folder.ToString() (and thus not const).

This search string means that you are unable to find folder annotations not being the first annotation in an annotation list, e.g. '@Interface @Folder("Interfaces").

Moreover, since the @ is contained in the search string, you will always get the annotationList context, not the individual annotationContext.

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

retailcoder · 2018-10-17T00:21:41Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

@@ -98,6 +145,31 @@ protected override void OnExecute(object parameter)
            }
        }

+        private string FolderNameFromFolderAnnotation(IParseTree parseTree, string folderAnnotation)
+        {
+            var searchAnnotation = parseTree.GetText();


Why not get the VBAParser.AnnotationContext node? AnnotationParametersFromContext(VBAParser.AnnotationContext context) (currently private, but also static) seems like it could be useful here.

MDoerner

You are doing well already, but you are not yet using the full work the parser has already done for you. You could avoid all the text search based problems from my comments by looking for contexts of type VBAParser.AnnotationContext only and using the annotationArg child of the first annotationArgList child to identify the folder.

There is one more nasty thing. As it stands, the code will update folder annotations that are not recognized, instead of adding a recognized one. Module annotations are only legal in the declaration section of the module (easy to handle) outside the member annotation sections of member variables (a bit harder to determine). For a first iteration, it might be acceptable though to not consider the member annotation sections.

MDoerner · 2018-10-17T07:04:06Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

+
+                string updatedModuleText;
+
+                var result = VBACodeStringParser.Parse(sourceText, t => t.startRule());


This will probably fail if there are precompiler directives in the module. Consider injecting an IStringParser into the command instead.

From what I on understood your I tried the code below and would only end up with Annotation as the result of annotationArg.parseTree.GetText(). What have I misunderstood?

var result = VBACodeStringParser.Parse(sourceText, t => t.startRule()); var antnList = VBACodeStringParser.Parse(result.parseTree.GetText(), x => x.annotationList()); var argListChild = VBACodeStringParser.Parse(antnList.parseTree.GetChild(0).GetText(), x => x.annotationArgList()); var annotationArg = VBACodeStringParser.Parse(argListChild.parseTree.GetChild(0).GetText(), x => x.annotationArg());

MDoerner · 2018-10-17T07:14:07Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

+            return searchAnnotation.Substring(startIndex, length);
+        }
+
+        private IParseTree TreeContainingFolderAnnotation(IParseTree containingTree, string folderAnnotation)


Why do a text search? You could either use the already existing AnnotationListener to get all annotation contexts and search in them for the one of type FolderAnnotation or build a custom visitor to return the folder annotation context, which is a faster option, I particular since you never have to enter the module body. (Folder annotations are only legal in the declaration section.)

This has the added benefit that you do not find things looking like folder annotations further in inside comments and that you get the actual AnnotationContext, which has a child for the parameter, which you just have to replace.

MDoerner · 2018-10-17T07:44:29Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

-                using (var components = project.VBComponents)
+                var extension = filename.Split('.').Last();
+
+                var sourceText = string.Join(Environment.NewLine, File.ReadAllLines(filename));


Could you either hide the file interaction behind an IFileHandler interface or extract the folder logic into another class. Otherwise, actually testing it is rather painful.

Is IFileHandler part of Rubberduck or .NET? I couldn't find it in either.

There is no such interface yet. I was making up a name for some interface hiding away the file interaction. You would either have to find one already existing in .Net or define your own.

MDoerner · 2018-10-17T07:47:41Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

+        private string FolderNameFromFolderAnnotation(IParseTree parseTree, string folderAnnotation)
+        {
+            var searchAnnotation = parseTree.GetText();
+            var folderNameEnclosedInQuotes = searchAnnotation.Contains('"');


Can't folder names themselves contain double quotes?

What if there is another annotation with a string parameter in the same annotation list?

I tested with an annotation of '@Folder("Folder"Foo"Bar") and it wasn't placed within a separate folder. It looks like double quotes aren't allowed.

I find that actually a bit surprising, but can confirm it. The other point still remains valid, though. An example would be @Description "foo" @Folder(bar).

MDoerner · 2018-10-17T07:54:20Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

+                var tempHelper = (CodeExplorerItemViewModel)parameter;
+                var updatedFolder = (parameter is CodeExplorerCustomFolderViewModel) ? tempHelper.Name : tempHelper.GetSelectedDeclaration().CustomFolder;
+
+                const string folderAnnotation = "'@Folder";


This search string means that you are unable to find folder annotations not being the first annotation in an annotation list, e.g. '@Interface @Folder("Interfaces").

MDoerner · 2018-10-17T07:57:49Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

+            var searchAnnotation = parseTree.GetText();
+            var folderNameEnclosedInQuotes = searchAnnotation.Contains('"');
+            var enclosingCharacter = folderNameEnclosedInQuotes ? '"' : ')';
+            var startIndex = searchAnnotation.IndexOf(folderAnnotation) + 1 + folderAnnotation.Length + (folderNameEnclosedInQuotes ? 1 : 0);


That will not work if the folder annotation is preceded by another annotation taking parameters in the same annotation list.

MDoerner · 2018-10-17T07:59:22Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

+                var tempHelper = (CodeExplorerItemViewModel)parameter;
+                var updatedFolder = (parameter is CodeExplorerCustomFolderViewModel) ? tempHelper.Name : tempHelper.GetSelectedDeclaration().CustomFolder;
+
+                const string folderAnnotation = "'@Folder";


Moreover, since the @ is contained in the search string, you will always get the annotationList context, not the individual annotationContext.

MDoerner · 2018-10-17T08:14:12Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

@@ -86,9 +89,53 @@ protected override void OnExecute(object parameter)

            foreach (var filename in _openFileDialog.FileNames)


This is not new in this PR, but while you are in here, could you please wrap this loop in a suspension action? As it currently stands, each module import will start a separate parse, potentially invalidating the project reference. (You can find an example in the TestEngine.)

Suspension action? I didn't see it in TestEngine.cs. This is something I'm not familiar with.

Hou can request go run something in a suspended parser state on the RubberDuckParserState. @bclothier introduced this a while ago and the test engine is using it to prevent a reparse while running tests.

It is not ghost important to add this right now. It could be added in aother PR.

MDoerner · 2018-10-17T08:28:00Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

                {
-                    components.Import(filename);
+                    updatedModuleText = $"{folderAnnotation}({updatedFolder}){Environment.NewLine}" + result.rewriter.GetText();


That might not be an appropriate position. The VBE will probably expect its wall of attributes to come first.

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

MDoerner

Generally, when using the parser on code, you do not call the method of parser corresponding to what part of the code you want on the end; you use the method for the rule matching the entire code you feed to the parser. (In your case, you have en entire module and, thus, should use startRule.) Otherwise, it will just fail.

To get a specific part, you generate the full parse tree and than inspect the tree to get the specific bit. To do this, you can define listener, attach it to a walker and walk the tree; you can define a visitor returning the information you want; you can go down the tree by hand, possibly aided by one of our extension methods.

Moreover, I would like to reiterate that the parser used by the VbaCodeStringParser is not suitable for arbitrary modules because it does not use a preprocessor. It will fail if there are precompiler directives in the module. Inject an appropriate IStringParser instead and use that to parse the module text.

MDoerner · 2018-10-19T07:09:22Z

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

+
+                string updatedModuleText;
+
+                var result = VBACodeStringParser.Parse(sourceText, t => t.moduleDeclarations());


That will generally fail because you are looking at an entire module, not only a module declaration section.

Moreover, as I stated before, to parse an entire module, inject an IStringParser. The static VbaCodeStringParser cannot handle precompiler directives.

MDoerner

That looks a lot better already.

However, there are still some safety issues for uncommon code..

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs

IvenBach · 2018-10-30T05:28:22Z

This is still using VBAParser. It needs to be updated with a more robust option to allow for conditional compilation as @MDoerner has already pointed out. I'll need some help with getting this understood and in working order. One step at a time.

Copied everything over from home to get work computer able to build.

…nto Issue4282_Import_module_folder_annotation

bclothier · 2019-01-03T23:43:09Z

Closing due to age/staleness. If anyone wants to pick it up, feel free to reopen.

IvenBach added the PR-Status: WIP Pull request is work-in-progress, more commits should be expected. label Oct 16, 2018

retailcoder reviewed Oct 16, 2018

View reviewed changes

Rubberduck.Core/UI/CodeExplorer/CodeExplorerControl.xaml Outdated Show resolved Hide resolved

retailcoder reviewed Oct 16, 2018

View reviewed changes

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs Outdated Show resolved Hide resolved

retailcoder reviewed Oct 17, 2018

View reviewed changes

MDoerner reviewed Oct 17, 2018

View reviewed changes

comintern reviewed Oct 17, 2018

View reviewed changes

Rubberduck.Core/UI/CodeExplorer/Commands/ImportCommand.cs Outdated Show resolved Hide resolved

MDoerner requested changes Oct 19, 2018

View reviewed changes

bclothier mentioned this pull request Oct 19, 2018

Self-Closing Pairs, round 2 (or is it round 3?) #4418

Merged

MDoerner reviewed Oct 25, 2018

View reviewed changes

IvenBach force-pushed the Issue4282_Import_module_folder_annotation branch 2 times, most recently from ed5eec9 to b729341 Compare October 31, 2018 01:04

Fix broken build at work --Squash

556a3ce

Copied everything over from home to get work computer able to build.

IvenBach force-pushed the Issue4282_Import_module_folder_annotation branch 3 times, most recently from 97dd065 to 556a3ce Compare October 31, 2018 17:32

IvenBach and others added 3 commits October 31, 2018 12:25

Remove unnecessary using directives

1708078

Parse with IStringParser

5ba83e3

Merge branch 'next' of https://github.com/rubberduck-vba/Rubberduck i…

bb4ff07

…nto Issue4282_Import_module_folder_annotation

MDoerner mentioned this pull request Nov 22, 2018

Filesystem management in RD #4533

Open

bclothier added the PR-Status: Conflicting PR can't be merged as it stands, conflicts must be resolved by the author. label Dec 29, 2018

bclothier closed this Jan 3, 2019

bclothier mentioned this pull request Feb 12, 2019

IO work needs to be abstracted out into an internal API #4794

Closed

IvenBach deleted the Issue4282_Import_module_folder_annotation branch May 9, 2019 19:36

IvenBach mentioned this pull request Aug 14, 2019

"Import Module" should add a @Folder annotation #4282

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue4282 import module folder annotation #4444

Issue4282 import module folder annotation #4444

IvenBach commented Oct 16, 2018

retailcoder Oct 16, 2018

MDoerner Oct 17, 2018

MDoerner Oct 17, 2018

retailcoder Oct 17, 2018

MDoerner left a comment

MDoerner Oct 17, 2018

IvenBach Oct 19, 2018

MDoerner Oct 17, 2018

MDoerner Oct 17, 2018

IvenBach Oct 19, 2018

MDoerner Oct 19, 2018

MDoerner Oct 17, 2018

MDoerner Oct 17, 2018

IvenBach Oct 18, 2018

MDoerner Oct 18, 2018

MDoerner Oct 17, 2018

MDoerner Oct 17, 2018

MDoerner Oct 17, 2018

MDoerner Oct 17, 2018

IvenBach Oct 19, 2018

MDoerner Oct 19, 2018

MDoerner Oct 17, 2018

MDoerner left a comment

MDoerner Oct 19, 2018

MDoerner left a comment

IvenBach commented Oct 30, 2018

bclothier commented Jan 3, 2019


		string updatedModuleText;

		var result = VBACodeStringParser.Parse(sourceText, t => t.startRule());

		@@ -86,9 +89,53 @@ protected override void OnExecute(object parameter)

		foreach (var filename in _openFileDialog.FileNames)

Issue4282 import module folder annotation #4444

Issue4282 import module folder annotation #4444

Conversation

IvenBach commented Oct 16, 2018

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

MDoerner left a 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

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

MDoerner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MDoerner left a comment

Choose a reason for hiding this comment

IvenBach commented Oct 30, 2018

bclothier commented Jan 3, 2019