Fix over-indexing of lists and restore string indexing by aparajit-pratap · Pull Request #9388 · DynamoDS/Dynamo

aparajit-pratap · 2019-01-08T20:28:47Z

Purpose

JIRA:
https://jira.autodesk.com/browse/DYN-995
https://jira.autodesk.com/browse/DYN-1056

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

@mjkkirschner

src/NodeServices/Attributes.cs

src/Libraries/DesignScriptBuiltin/Builtin.cs

aparajit-pratap · 2019-01-09T23:01:31Z

test/Engine/ProtoTest/Associative/MicroFeatureTests.cs

            thisTest.Verify("r", 5);
        }

-        [Test, Category("Failure")]


This test doesn't make any sense.

mjkkirschner · 2019-01-09T23:35:02Z

src/Engine/ProtoCore/RuntimeStatus.cs

            AccessViolation,
            AmbiguousMethodDispatch,
-            AurgumentIsNotExpected,
+            ArgumentIsNotExpected,


@aparajit-pratap I don't think we can change these now, they are public. 😢 maybe just add a todo?

mjkkirschner · 2019-01-09T23:40:01Z

@aparajit-pratap looks good except I don't think we can fix the enum names now.

aparajit-pratap · 2019-01-10T04:15:23Z

@mjkkirschner these are changes in ProtoCore. I'm pretty sure no users/clients use any public stuff from ProtoCore (other than AST's). Or are you saying this will cause binary incompatibility issues?

mjkkirschner · 2019-01-10T07:27:50Z

Binary incompatibility is what I’m worried about.

…

On Jan 9, 2019, at 11:15 PM, aparajit-pratap ***@***.***> wrote: @aparajit-pratap these are changes in ProtoCore. I'm pretty sure no users use any public stuff from ProtoCore. Or are you saying this will cause binary incompatibility issues? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mjkkirschner · 2019-01-10T20:48:19Z

src/Engine/ProtoCore/Parser/AssociativeAST.cs

        public string ObsoleteMessage { get; protected set; }
        public bool IsObsolete { get { return !string.IsNullOrEmpty(ObsoleteMessage); } }
        public bool IsLacingDisabled { get; protected set; }
+        public bool AllowArrayPromotion { get; protected set; } = true;


@aparajit-pratap should this also be added to the imperative AST methodAttributes class - does that exist?

Strictly speaking this class doesn't have anything to do with Associative AST's and should not be in this file but that's a consideration for another day.

mjkkirschner · 2019-01-10T20:48:48Z

@aparajit-pratap looks good to me - one question about imperative behavior vs associative?

aparajit-pratap · 2019-01-11T00:29:53Z

Thanks @mjkkirschner, merging.

* fix overindexing of lists * add null checks * fixes * revert assemblysharedinfo * fix tests * reverting unchanged file * turn on previously failing tests that pass after fix * revert public to protected * revert unchanged file * fix warning messages

QilongTang · 2019-01-11T03:06:03Z

src/Engine/ProtoCore/FFI/CLRFFIFunctionPointer.cs

 using ProtoCore.Properties;
 using ProtoCore.Exceptions;
+using IndexOutOfRangeException = DesignScript.Builtin.IndexOutOfRangeException;
+using KeyNotFoundException = DesignScript.Builtin.KeyNotFoundException;


Can you make sure the usings are sorted?

QilongTang · 2019-01-11T03:08:10Z

src/Engine/ProtoCore/FFI/CLRObjectMarshaler.cs

            CLRObjectMarshaler marshaller = null;
            if (!mObjectMarshlers.TryGetValue(sender, out marshaller))
-                throw new KeyNotFoundException();
+                throw new System.Collections.Generic.KeyNotFoundException();


hmm This file has System.Collections.Generic using already I believe

this is to be explicit because we also have a key not found exception.

@mjkkirschner Then should we rename our own exception in a different task?

please a different task. ours is
DesignScript.Builtin.KeyNotFoundException
@aparajit-pratap is aliasing them above.

src/Libraries/DesignScriptBuiltin/Builtin.cs

* Cherry-picking #9388 into 2.0.3 * Cherry-picking #9559 into 2.0.3 * Cherry-picking #9578 into 2.0.3 * cherry-picking #9632 into 2.0.3 * cherry-picking #9408 into 2.0.3 * cherry-picking #9441 into 2.0.3 * Adding gradient.png for the Test_PerforationsByImage test. This was missed while cherrypicking #9441 * Removing DSCoreDataTests.cs as this was the test fixture was introduced in a different commit and is not needed here.

aparajit-pratap added 2 commits January 8, 2019 15:26

fix overindexing of lists

948ebc7

add null checks

6d0e51f

mjkkirschner reviewed Jan 8, 2019

View reviewed changes

src/NodeServices/Attributes.cs Show resolved Hide resolved

mjkkirschner reviewed Jan 8, 2019

View reviewed changes

src/Libraries/DesignScriptBuiltin/Builtin.cs Show resolved Hide resolved

aparajit-pratap added 3 commits January 8, 2019 16:59

fixes

cfbbcc6

revert assemblysharedinfo

11be265

fix tests

51b7898

aparajit-pratap mentioned this pull request Jan 9, 2019

Regression: String indexing broken in Dynamo 2.0.x #9383

Closed

aparajit-pratap added 3 commits January 8, 2019 23:58

reverting unchanged file

8ff5436

turn on previously failing tests that pass after fix

28d6b18

revert public to protected

69bb364

aparajit-pratap commented Jan 9, 2019

View reviewed changes

revert unchanged file

27123f2

mjkkirschner reviewed Jan 9, 2019

View reviewed changes

fix warning messages

a41412c

aparajit-pratap changed the title ~~Fix overindexing of lists~~ Fix over-indexing of lists and restore string indexing Jan 10, 2019

mjkkirschner reviewed Jan 10, 2019

View reviewed changes

aparajit-pratap merged commit e151cb9 into DynamoDS:master Jan 11, 2019

aparajit-pratap deleted the arrayIndexing branch January 11, 2019 00:30

aparajit-pratap mentioned this pull request Jan 11, 2019

Fix over-indexing of lists and restore string indexing (#9388) #9411

Merged

QilongTang reviewed Jan 11, 2019

View reviewed changes

src/Libraries/DesignScriptBuiltin/Builtin.cs Show resolved Hide resolved

aparajit-pratap mentioned this pull request Jan 16, 2019

fix to List.CartesianProduct node #9431

Merged

7 tasks

reddyashish pushed a commit to reddyashish/Dynamo that referenced this pull request May 21, 2019

Cherry-picking DynamoDS#9388 into 2.0.3

fd04060

reddyashish mentioned this pull request May 21, 2019

Cherry-pick language changes into 2.0.3 branch for Refinery #9730

Merged

7 tasks

Conversation

aparajit-pratap commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Jan 9, 2019

Uh oh!

aparajit-pratap commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Jan 10, 2019 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Jan 10, 2019

Uh oh!

aparajit-pratap commented Jan 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aparajit-pratap commented Jan 8, 2019 •

edited

Loading

aparajit-pratap commented Jan 10, 2019 •

edited

Loading

mjkkirschner Jan 11, 2019 •

edited

Loading