Improve the autocomplete feature of the builtin python IDE #9402

alfarok · 2019-01-10T14:50:25Z

Background

Picking up the awesome work originally completed by @dimven in #8420

Addresses instabilities when using the built in IDE with large scripts and for extended periods of time
Revised current autocomplete implementation to avoid several exceptions
Improve the type guessing as much as possible

The type guessing should be relatively accurate for simple statements. It is difficult to guarantee 100% coverage for complicated variable definitions as it would require the luxury of running in the same scope as the actual script or pre-executing the entire script.

Purpose

This PR does the following:

addresses comments in Improve the autocomplete feature of the builtin python IDE #8420
attempts to avoid any API breaking changes
mark old methods Obsolete while making new properties/methods
private/internal
points python standard library references to the new bundled Dynamo Core location
comments/formatting

Testing

all unit tests passing (regex is tested in CodeCompletionTests.cs)

Examples

The examples in #8420 are all still valid.

ProtoGeometry

Python Standard Library

Declarations

Check these if you believe they are true

The code base is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@dimven @mjkkirschner @QilongTang

FYIs

@Racel

alfarok · 2019-01-10T16:58:11Z

Looks like there are some test failures on the self-serve, looking into them now

…on_Autocomplete

mjkkirschner · 2019-01-10T17:47:16Z

src/Libraries/PythonNodeModelsWpf/IronPythonCompletionProvider.cs

+                if (string.IsNullOrEmpty(dynamoCoreDir))
+                {
+                    // Gather executing assembly location
+                    string nodePath = Assembly.GetExecutingAssembly().Location;


this seems odd - I can see this returning odd results on some systems, could it be worth trying to get access to the pathManager or pathResolver which has all of these paths?

I tested in Sandbox and Revit without issues but I agree getting a path and stepping back is a bit strange. I assumed the Nodes folder was always relative to the Root directory. Wouldn't I need access to the Dynamo model to use the pathManager?

If we do make a change we should probably do it here as well, although there is no navigating in this scenario.

https://github.com/DynamoDS/Dynamo/blob/master/src/Libraries/DSIronPython/IronPythonEvaluator.cs#L44

This class is initialized in ScriptEditor, you can pass in dynamoViewModel.Model.PathManager.DynamoCoreDirectory when initializing instead of getting this info in the getter, it is a dup of what we have in PathManager.

Adding a new constructor will do the job for you

public IronPythonCompletionProvider(string dynamoCoreDir ) { pythonLibDir= [you logic of getting the dir from dynamoCoreDir ] IronPythonCompletionProvider(); }

^awesome, thanks @QilongTang!

I have to duplicate the logic from the original IronPythonCompletionProvider() constructor and mark the original Obsolete correct? I didn't think you could call a constructor from within a constructor of the same class like that without constructor chaining

@alfarok you don't NEED to obsolete the other constructor if it should be called, I don't really see why you need to chain the constructors at all but you can if thats useful.

I would also consider adding the std library path as a parameter, (maybe optional?)

but I dont think you can call one constructor from inside another to any great effect since you dont actually explicitly return from a constructor in c#

@mjkkirschner I was thinking that's why I would need to duplicate everything from the original since it can't be called from within the new constructor but I wasn't sure

ahh, I see, it's a big constructor, yeah you can use chaining to call the other constructor with the :this() syntax.

mjkkirschner · 2019-01-10T17:49:27Z

src/Libraries/PythonNodeModelsWpf/IronPythonCompletionProvider.cs

+                // Try to load Python Standard Library Type for autocomplete
+                try
+                {
+                    var pyLibImports = String.Format("import sys\nsys.path.append(r'{0}')\n", pythonStdLib);


does this still need to be done?

yes it is required to load the autocomplete for the python std modules. It is also being done for ProtoGeometry which is included in the template by default.

mjkkirschner · 2019-01-10T17:52:06Z

src/Libraries/PythonNodeModelsWpf/IronPythonCompletionProvider.cs

            return paramMatches;
+        }
+
+        public List<string> findClrReferences(string code)


needs a summary tag

this along with one other method above can actually be private, will replace them now

alfarok · 2019-01-10T19:35:56Z

After pulling in the latest changes from master, down to 7 test failures that need to be addressed

1) Test Failure : DynamoPythonTests.CodeCompletionTests.CanImportLibrary
     Expected: 1
  But was:  2

at DynamoPythonTests.CodeCompletionTests.CanImportLibrary()

2) Test Failure : DynamoPythonTests.CodeCompletionTests.CanImportSystemCollectionsLibraryAndGetCompletionData
     Expected: True
  But was:  False

at DynamoPythonTests.CodeCompletionTests.CanImportSystemCollectionsLibraryAndGetCompletionData()

3) Test Failure : DynamoPythonTests.CodeCompletionTests.CanImportSystemLibraryAndGetCompletionData
     Expected: 1
  But was:  2

at DynamoPythonTests.CodeCompletionTests.CanImportSystemLibraryAndGetCompletionData()

4) Test Failure : DynamoPythonTests.CodeCompletionTests.CanMatchAllVariablesSingleLine
     Expected: 1
  But was:  0

at DynamoPythonTests.CodeCompletionTests.CanMatchAllVariablesSingleLine()

5) Test Failure : DynamoPythonTests.CodeCompletionTests.CanMatchAllVariableTypes
     Expected: 5
  But was:  0

at DynamoPythonTests.CodeCompletionTests.CanMatchAllVariableTypes()

6) Test Failure : DynamoPythonTests.CodeCompletionTests.CanMatchImportSystemAndLoadLibraryAndWithComment
     Expected: True
  But was:  False


7) Test Failure : DynamoPythonTests.CodeCompletionTests.DuplicateCallsToImportShouldBeFine
     Expected: 1
  But was:  2

at DynamoPythonTests.CodeCompletionTests.DuplicateCallsToImportShouldBeFine()

QilongTang · 2019-01-10T20:37:52Z

src/Libraries/PythonNodeModelsWpf/IronPythonCompletionProvider.cs

+        /// <summary>
+        /// Cache storing a reference to the Dynamo Core directory
+        /// </summary>
+        private static string dynamoCoreDir { get; set; }


I think we can remove this property, see comments below

@alfarok I agree with @QilongTang - that seems like a good place to pass the references you need.

alfarok · 2019-01-11T02:25:17Z

src/Libraries/PythonNodeModelsWpf/IronPythonCompletionProvider.cs

+        /// Class constructor
+        /// </summary>
+        [Obsolete("Use additional constructor passing the Dynamo Core Directory to enable Python Std Lib autocomplete.")]
+        public IronPythonCompletionProvider() : this("")


@mjkkirschner @QilongTang is this an API breaking change? The original constructor calls the new constructor passing an empty string as the dynamoCoreDir param. If this is null or empty the standard lib autocomplete is not loaded

I'm not sure! It doesn't change the signature, so it's not immediately obvious that it is a break.

I dont think it breaks,

Function signature does not change

from your code, if it is called without a param, the behavior is still same with before

alfarok · 2019-01-11T15:27:18Z

src/Libraries/PythonNodeModelsWpf/IronPythonCompletionProvider.cs

-            RegexToType.Add(arrayRegex, typeof(List));
-            RegexToType.Add(dictRegex, typeof(PythonDictionary));
+            // Special case for python variables defined as null
+            ImportedTypes["None"] = null;


@mjkkirschner @QilongTang This causes 3 old tests to fail because a new ImportedType is being added so completionProvider.ImportedTypes.Count has stepped by 1 for:

CanImportLibrary()

CanImportSystemLibraryAndGetCompletionData()

DuplicateCallsToImportShouldBeFine()

Still working through the remaining 4

@mjkkirschner this is the other type that is added by default which is why several tests stepped by 1

mjkkirschner · 2019-01-11T19:47:03Z

src/Libraries/PythonNodeModelsWpf/IronPythonCompletionProvider.cs

@@ -47,6 +44,11 @@ public ScriptScope Scope
            set { scope = value; }
        }

+        /// <summary>
+        /// Cache storing a reference to the Dynamo Core directory


is this comment correct? seems odd.

mjkkirschner · 2019-01-11T19:47:53Z

src/Libraries/PythonNodeModelsWpf/IronPythonCompletionProvider.cs

+            // Determine if the Python Standard Library is available in the given context
+            if(!String.IsNullOrEmpty(dynamoCoreDir))
+            {
+                pythonLibDir = dynamoCoreDir + @"\IronPython.StdLib.2.7.8";


@alfarok don't do this - instead use System.Path.Combine - it will work cross platform.

mjkkirschner · 2019-01-11T19:48:59Z

test/Libraries/DynamoPythonTests/CodeCompletionTests.cs

@@ -237,7 +237,7 @@ public void CanImportLibrary()
            var completionProvider = new IronPythonCompletionProvider();
            completionProvider.UpdateImportedTypes(str);

-            Assert.AreEqual(1, completionProvider.ImportedTypes.Count);
+            Assert.AreEqual(2, completionProvider.ImportedTypes.Count);


what are the other types?

mjkkirschner · 2019-01-11T20:04:08Z

@alfarok looks good so far, will review again when you fix the other tests. Two comments:

don't build up the paths manually - use the Path.Combine functions for this so this will work on other platforms when run under mono or dot net core.
what new imported types are present during the test - where are they coming from?

dimven · 2019-01-13T20:10:59Z

@alfarok thanks for finishing this up, the changes look great! My only comment is on the GetName method because with the current implementation the following cases fail:

a=Line. - fails because there is no whitespace
a =Line - fails with an exception because the equals sign is not discarded

I recommend the following minor change:

string GetName(string text)
        {
            text = text.Replace("\t", "   ")
                       .Replace("\n", " ")
                       .Replace("\r", " ");
            int startIndex1 = text.LastIndexOf(' ');
            int startIndex2 = text.LastIndexOf('=');
            return text.Substring(Math.Max(startIndex1, startIndex2) + 1).Trim('.').Trim('(');
        }

At this point I'm stating to think it's probably better to just replace this whole method with some regex as well.

mjkkirschner · 2019-01-14T01:08:16Z

src/Libraries/PythonNodeModelsWpf/IronPythonCompletionProvider.cs

@@ -205,7 +205,7 @@ public IronPythonCompletionProvider(string dynamoCoreDir)
            // Determine if the Python Standard Library is available in the given context
            if(!String.IsNullOrEmpty(dynamoCoreDir))
            {
-                pythonLibDir = dynamoCoreDir + @"\IronPython.StdLib.2.7.8";
+                pythonLibDir = Path.Combine(dynamoCoreDir + @"\IronPython.StdLib.2.7.8");


@alfarok this is not right - it should be: Path.Combine(dynamoCoreDir, "IronPython.StdLib.2.7.8");

@mjkkirschner nice catch, fixing now

alfarok · 2019-01-14T15:54:58Z

src/Libraries/PythonNodeModelsWpf/IronPythonCompletionProvider.cs

+                var previousTries = 0;
+                badStatements.TryGetValue(statement, out previousTries);
+                // TODO - why is this 3?  Should this be a constant?
+                if (previousTries > 3)


@dimven Thanks for taking a look, I think we are pretty close! I will take a closer look at the GetName method as it may be related to some of the remaining 4 test failures. It is also possible the tests are using old methods that aren't updated with the new logic.

Do you happen to remember the significance of this 3 above? Possibly related to the length of these known assemblies?

private static string[] knownAssemblies = { "mscorlib", "RevitAPI", "RevitAPIUI", "ProtoGeometry" };

I also don't see any references to GetName in the current implementation which is why I marked it Obsolete, but I may be missing something

Ah true. Didn't notice you split it in two methods.
The knownAssemblies is used in TryGetTypeFromFullName method, which acts as a short-circuit around firing up the python interpreter for any full name types that were already preloaded.

alfarok · 2019-01-15T21:07:13Z

Down to 1 failure but I am skeptical about a couple others that are simply checking the imported types count, doing a little more digging.

CanMatchImportSystemAndLoadLibraryAndWithComment()

mjkkirschner · 2019-01-17T02:04:33Z

test/Libraries/DynamoPythonTests/CodeCompletionTests.cs

@@ -293,6 +292,7 @@ public void CanMatchImportSystemLibraryWithComment()
            var matches = IronPythonCompletionProvider.FindBasicImportStatements(str);

            Assert.AreEqual(1, matches.Count);
+            Assert.IsTrue(matches.ContainsKey("System"));


you could use sequence equal here to be more specific with the test results

This was the only location we strictly checked the count without verifying the matching key didn't change. Should I update this for all instances?

alfarok · 2019-01-17T02:15:59Z

@mjkkirschner I think this is ready for a final review, all tests should be passing. We now handle the scenario in which an import statement ends in . which was causing the last failure. Running the self-serve one last time now to confirm.

alfarok · 2019-01-17T15:09:26Z

All unit tests are now passing on the self-serve CI

…l expected defaults

mjkkirschner · 2019-01-18T01:22:43Z

test/Libraries/DynamoPythonTests/CodeCompletionTests.cs

+                "None"
+            };
+
+            foreach (var type in importedTypes)


this seems to reimplement sequence equals:
https://docs.microsoft.com/en-us/dotnet/api/system.linq.enumerable.sequenceequal?view=netframework-4.7.2 ?

any reason not to just use it?

Should be good now

dimven · 2019-01-18T10:49:56Z

@alfarok would it be possible to fit in one more usability update for the 2.1 release? If so, I added a secondary generic autocomplete that pops up automatically after every 2nd character and suggest the built-in python methods and the most common "words" used in the script above:

That helps me greatly reduces the chance for spelling mistakes and speeds up writing.

alfarok · 2019-01-18T15:22:27Z

@dimven I want to get this work merged since the original PR has been open for while and already has quite a few changes. If you want to open a new PR with those changes we can take a look (much faster than the last PR 😁), it definitely seems like a nice to have feature.

Unfortunately, these changes will all be going into 2.2 because the 2.1 branch has already been cut and gone through preliminary testing. I am not 100% sure on this but I think the plan moving forward is to release updates to Dynamo Core more frequently so there should be less time between these releases.

Thanks again for all your efforts, these are some really nice enhancements and I am sure the community will be very pleased!

alfarok · 2019-01-18T17:34:37Z

got the LGTM from @mjkkirschner. @dimven Any chance you have a semi-long python script that doesn't touch the Revit API that you could share? Doesn't have to do anything fancy but after speaking with Mike we thought it would be a good to add a test catching any performance degradation in the future with parsing long scripts.

dimven · 2019-02-12T12:21:14Z

@alfarok , sorry for not getting back to you in time. Unfortunately, I didn't have anything non-revit related to test this but maybe one of the sample from the primers will be perfect for this: http://primer.dynamobim.org/13_Best-Practice/datasets/13-2/RoofDrainageSim.zip

It has a semi-long python script at the end of the graph.

dimven · 2019-02-12T12:34:43Z

@alfarok , sorry for not getting back to you in time. Unfortunately, I didn't have anything non-revit related to test this but maybe one of the sample from the primers will be perfect for this: http://primer.dynamobim.org/13_Best-Practice/datasets/13-2/RoofDrainageSim.zip

It has a semi-long python script at the end of the graph.

dimven and others added 10 commits January 7, 2019 10:13

Pull Dimitar'soriginal PR commits

3ce8c87

address existing PR comments

e3d2396

address python std lib path

ec6a098

cleanup

3db1edf

address API changes

77b2dc4

autocomplete for standard lib

844d2b3

comments and cleanup

5d56ebf

InternalsVisibleTo not required until refactor

03660c5

restore unnecessary change

5ab51a2

remove unnecessary delta

3372ea7

alfarok added the PTAL Please Take A Look 👀 label Jan 10, 2019

alfarok mentioned this pull request Jan 10, 2019

Improve the autocomplete feature of the builtin python IDE #8420

Closed

7 tasks

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into Pyth…

c1ad4e1

…on_Autocomplete

mjkkirschner reviewed Jan 10, 2019

View reviewed changes

change access modifiers for new methods

7efab03

QilongTang reviewed Jan 10, 2019

View reviewed changes

get dynamo core path via the model

20c2b69

alfarok commented Jan 11, 2019

View reviewed changes

cleanup obsolete redundancy

945e659

alfarok commented Jan 11, 2019

View reviewed changes

step count with new imported type

28f9bb2

mjkkirschner reviewed Jan 11, 2019

View reviewed changes

update summary tag

321f089

use Path.Combine

23b6b27

mjkkirschner reviewed Jan 14, 2019

View reviewed changes

alfarok commented Jan 14, 2019

View reviewed changes

alfarok added 4 commits January 14, 2019 12:04

fix path combine

68e99ed

pull Mike K's changes

e7c5bcb

add regions to large cs file

55c5ad0

lots of cleanup!

1e78023

fix final failing test

dea9652

mjkkirschner reviewed Jan 17, 2019

View reviewed changes

verify collections of imported types with more than 1 type contain al…

30eea8a

…l expected defaults

mjkkirschner reviewed Jan 18, 2019

View reviewed changes

switch to SequenceEquals for collection comparison

6d7268e

alfarok added LGTM Looks good to me and removed PTAL Please Take A Look 👀 labels Jan 18, 2019

alfarok merged commit c9ee999 into DynamoDS:master Jan 18, 2019

johnpierson mentioned this pull request Feb 11, 2019

[Bug] Python node: Specifically importing 'FilteredElementCollector' causes hang #8931

Closed

Improve the autocomplete feature of the builtin python IDE #9402

Improve the autocomplete feature of the builtin python IDE #9402

Conversation

alfarok commented Jan 10, 2019 • edited Loading

Background

Purpose

Testing

Examples

Declarations

Reviewers

FYIs

alfarok commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QilongTang Jan 10, 2019 • edited Loading

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

alfarok commented Jan 10, 2019

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

mjkkirschner commented Jan 11, 2019

dimven commented Jan 13, 2019 • edited Loading

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

dimven Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

alfarok commented Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfarok commented Jan 17, 2019

alfarok commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimven commented Jan 18, 2019

alfarok commented Jan 18, 2019

alfarok commented Jan 18, 2019

dimven commented Feb 12, 2019

dimven commented Feb 12, 2019

alfarok commented Jan 10, 2019 •

edited

Loading

QilongTang Jan 10, 2019 •

edited

Loading

dimven commented Jan 13, 2019 •

edited

Loading

dimven Jan 15, 2019 •

edited

Loading

alfarok commented Jan 15, 2019 •

edited

Loading