Get item from list with negative index (Wishlist #187)

[tinrobot2000]

Purpose

Update to List.GetItemAtIndex to allow negative index (get from right to left) retrieval https://github.com/DynamoDS/DynamoWishlist/issues/187

Declarations

Check these if you believe they are true

• [ ] The codebase 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.
• [ ] This PR modifies some build requirements and the readme is updated
• [ ] This PR contains no files larger than 50 MB

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

[github-actions[bot]]

UI Smoke Tests

Test: success. 2 passed, 0 failed. TestComplete Test Result Workflow Run: UI Smoke Tests Check: UI Smoke Tests - net8.0

[QilongTang]

hi @tinrobot2000 Thanks a lot for proposing this PR, my team will review and make a decision soon on this. Good work on addressing wishlist issue!

[reddyashish]

@aparajit-pratap @mjkkirschner Do you think this is as per design to not gte the negative index from the list items?

[mjkkirschner]

@aparajit-pratap @mjkkirschner Do you think this is as per design to not gte the negative index from the list items?

If you checkout the builtin implementation of Get indexing it does support negative indices: https://github.com/DynamoDS/Dynamo/blob/49a3f7c3025d350405d9af28b0e5c3ab8ad59b3a/src/Libraries/DesignScriptBuiltin/Builtin.cs#L33

I think this would be an okay change but it requires the following changes IMO.

1. a test
2. consider performance impact, 1 if statement probably is not too bad.
3. I don't see any reason to raise the IndexOutOfRangeException, the indexing operation is going to do that anyway, what am I missing?

[QilongTang]

@aparajit-pratap @mjkkirschner Do you think this is as per design to not gte the negative index from the list items?

If you checkout the builtin implementation of Get indexing it does support negative indices:

https://github.com/DynamoDS/Dynamo/blob/49a3f7c3025d350405d9af28b0e5c3ab8ad59b3a/src/Libraries/DesignScriptBuiltin/Builtin.cs#L33

I think this would be an okay change but it requires the following changes IMO.

1. a test
2. consider performance impact, 1 if statement probably is not too bad.
3. I don't see any reason to raise the IndexOutOfRangeException, the indexing operation is going to do that anyway, what am I missing?

Hi @mjkkirschner I think when index is too large or too negative to handle, it could be out of range so exception is only needed in that case. I am adding unit tests for those cases as well.

[mjkkirschner]

Hi @mjkkirschner I think when index is too large or too negative to handle, it could be out of range so exception is only needed in that case. I am adding unit tests for those cases as well.

yeah I'm still not getting it - when you do

var x= int[1]
var y = x[10000000]

that will throw an out of range exception, you don't have to detect it yourself first...