Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List.GetItemAtIndex should work with negative indices. #187

Closed
JacobSmall opened this issue Jan 4, 2021 · 5 comments
Closed

List.GetItemAtIndex should work with negative indices. #187

JacobSmall opened this issue Jan 4, 2021 · 5 comments
Labels
Easy first task Easy first task for new team members Nodes Nodal improvement UX UX improvement

Comments

@JacobSmall
Copy link

What improvement/feature would you like to see in Dynamo?

Negative indicies less than the length of the index should pull from the end of the list.

How would you see this improvement/feature working?

image this would return the last item in the list.

@Amoursol Amoursol added Easy first task Easy first task for new team members Nodes Nodal improvement UX UX improvement labels Jan 4, 2021
tinrobot2000 added a commit to tinrobot2000/Dynamo that referenced this issue May 10, 2024
@avidit avidit closed this as completed May 28, 2024
@andydandy74
Copy link

@Amoursol @JacobSmall Just noticed the PR for this issue. Have the repercussions on existing graphs been considered? The GetItemAtIndex node is often used in conjunction with the IndexOf node. Currently that node will return -1 for items not found in the list. So what I envision happening in existing graphs is that instead of returning null for items not in the list we will now see the last item in the list returned in such cases:
grafik
Would it perhaps make sense to have the IndexOf node return null instead of -1 for items not found in the list in the future?

@QilongTang
Copy link

@andydandy74 You are right on the expected result
image

@Amoursol I think the proposed change on IndexOf node makes more sense after the negative index support, what do you think?

@andydandy74
Copy link

andydandy74 commented May 30, 2024

Proposed change see DynamoDS/Dynamo#15268

@Amoursol
Copy link
Contributor

Amoursol commented May 30, 2024

@QilongTang Returning null works for me!

@mjkkirschner
Copy link
Member

mjkkirschner commented May 30, 2024

@QilongTang @avidit - perhaps it's worth taking a sec to consider other approaches like adding a new node with this behavior.
We know that returning null and passing it downstream creates its own issues.
Additionally it seems like we're missing some test cases that combine using the two nodes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy first task Easy first task for new team members Nodes Nodal improvement UX UX improvement
Projects
Status: Done
Development

No branches or pull requests

6 participants